Copilot commented on code in PR #13047:
URL: https://github.com/apache/gluten/pull/13047#discussion_r4072330197
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala:
##########
@@ -37,13 +38,42 @@ 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 {
+ case Literal(null, IntegerType) => true
+ case Literal(scale: Int, IntegerType) =>
+ if (scale < -400 || scale > 400) {
+ logDebug(
+ s"Bround scale $scale is outside the native [-400, 400]
interval; " +
+ "falling back to Spark.")
+ false
+ } else if (
+ scale != 0 &&
+ (round.child.dataType == FloatType || round.child.dataType ==
DoubleType) &&
+ !Properties.isJavaAtLeast("21")
+ ) {
Review Comment:
The native scale bounds (-400/400) and the JVM cutoff (\"21\") are
hard-coded here and duplicated across tests/docs in this PR. Please centralize
these as named constants (e.g., in a companion object/shared config) and reuse
them in validator + tests to prevent future drift (e.g., suite expectations
becoming inconsistent with validation).
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala:
##########
@@ -37,13 +38,42 @@ 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 {
+ case Literal(null, IntegerType) => true
+ case Literal(scale: Int, IntegerType) =>
+ if (scale < -400 || scale > 400) {
+ logDebug(
+ s"Bround scale $scale is outside the native [-400, 400]
interval; " +
+ "falling back to Spark.")
+ false
+ } else if (
+ scale != 0 &&
+ (round.child.dataType == FloatType || round.child.dataType ==
DoubleType) &&
+ !Properties.isJavaAtLeast("21")
+ ) {
+ logDebug(
+ "Floating-point bround with nonzero scale requires Java 21 or
later " +
+ "for matching decimal conversion; falling back to Spark.")
Review Comment:
The debug messages use inconsistent capitalization ('Bround' vs 'bround').
For easier log searching/grepping and consistency with the function name,
consider standardizing these messages to consistently use 'bround' (lowercase)
or 'BRound' (class name).
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala:
##########
@@ -37,13 +38,42 @@ 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 {
+ case Literal(null, IntegerType) => true
+ case Literal(scale: Int, IntegerType) =>
+ if (scale < -400 || scale > 400) {
+ logDebug(
+ s"Bround scale $scale is outside the native [-400, 400]
interval; " +
+ "falling back to Spark.")
Review Comment:
The debug messages use inconsistent capitalization ('Bround' vs 'bround').
For easier log searching/grepping and consistency with the function name,
consider standardizing these messages to consistently use 'bround' (lowercase)
or 'BRound' (class name).
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala:
##########
@@ -37,13 +38,42 @@ 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 {
+ case Literal(null, IntegerType) => true
+ case Literal(scale: Int, IntegerType) =>
+ if (scale < -400 || scale > 400) {
+ logDebug(
+ s"Bround scale $scale is outside the native [-400, 400]
interval; " +
+ "falling back to Spark.")
+ false
+ } else if (
+ scale != 0 &&
+ (round.child.dataType == FloatType || round.child.dataType ==
DoubleType) &&
+ !Properties.isJavaAtLeast("21")
+ ) {
+ logDebug(
+ "Floating-point bround with nonzero scale requires Java 21 or
later " +
+ "for matching decimal conversion; falling back to Spark.")
+ false
+ } else {
+ true
+ }
+ case _ =>
+ logDebug("Bround scale must be a folded INTEGER literal; falling
back to Spark.")
Review Comment:
The debug messages use inconsistent capitalization ('Bround' vs 'bround').
For easier log searching/grepping and consistency with the function name,
consider standardizing these messages to consistently use 'bround' (lowercase)
or 'BRound' (class name).
--
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]
