luis4a0 commented on code in PR #13047:
URL: https://github.com/apache/gluten/pull/13047#discussion_r4072904373
##########
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:
Addressed in
https://github.com/apache/gluten/commit/00f8c9f9e57c6bee91c3c7c4b773732e7146eca0.
The existing `VeloxValidatorApi` companion now defines the scale bounds and
minimum floating-point Java version; validation, diagnostics, and backend tests
reuse them. A separate test pins the qualified values, and the docs link their
definitions. The shared Spark suite deliberately retains independent contract
literals rather than importing a Velox-only class, preserving ClickHouse
compilation; it now also tests both inclusive endpoints. Both Java 17 and 21
passed 9 validator tests, 2 transformer tests, and all 9 integration tests.
Eligibility and the dependency pin are unchanged.
##########
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:
Fixed in
https://github.com/apache/gluten/commit/00f8c9f9e57c6bee91c3c7c4b773732e7146eca0.
All three fallback diagnostics now consistently use lowercase `bround`; the
range and Java-version text also reuse the named compatibility constants.
##########
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:
Addressed together with the duplicate spelling comments in
https://github.com/apache/gluten/commit/00f8c9f9e57c6bee91c3c7c4b773732e7146eca0.
This floating-point diagnostic and both other fallback diagnostics
consistently use lowercase `bround`.
##########
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:
Fixed in
https://github.com/apache/gluten/commit/00f8c9f9e57c6bee91c3c7c4b773732e7146eca0.
The folded-literal fallback diagnostic now uses lowercase `bround`, matching
the other fallback messages.
--
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]
