minni31 commented on code in PR #13214:
URL: https://github.com/apache/gluten/pull/13214#discussion_r4479629600
##########
backends-velox/src/test/scala/org/apache/gluten/functions/ScalarFunctionsValidateSuite.scala:
##########
@@ -197,6 +197,62 @@ class ScalarFunctionsValidateSuite extends
FunctionsValidateSuite {
}
}
+ test("shiftrightunsigned") {
+ withTempPath {
+ path =>
+ withTempView("shift_input") {
+ Seq[(Integer, java.lang.Long, Integer)](
+ (8, 8L, 1),
+ (-1, -1L, 1),
+ (Int.MinValue, Long.MinValue, 1),
+ (Int.MinValue, Long.MinValue, 31),
+ (-1, -1L, 32),
+ (-1, -1L, 33),
+ (-1, -1L, 63),
+ (-1, -1L, 64),
+ (-1, -1L, 65),
+ (-1, -1L, -1),
+ (Int.MinValue, Long.MinValue, Int.MaxValue),
+ (null, null, 1),
+ (1, 1L, null)
+ )
+ .toDF("int_value", "long_value", "shift")
+ .write
+ .parquet(path.getCanonicalPath)
+
+
spark.read.parquet(path.getCanonicalPath).createOrReplaceTempView("shift_input")
+ runQueryAndCompare(
+ """
+ |SELECT
+ | shiftrightunsigned(int_value, shift),
+ | shiftrightunsigned(long_value, shift)
+ |FROM shift_input
+ |""".stripMargin) {
+ df =>
+ checkGlutenPlan[ProjectExecTransformer](df)
+ assert(
+
!getExecutedPlan(df).exists(_.isInstanceOf[ColumnarPartialProjectExec]),
+ "Unsigned shifts must not use partial JVM projection")
Review Comment:
Agreed that including the offending nodes would improve diagnostics. The
current commit adds an explicit partial-JVM-projection failure message and
closes the coverage gap, but it does not print the matching nodes or plan
fragments. That diagnostic enhancement is not implemented in the current patch.
##########
docs/backends/velox/velox-backend-scalar-function-support.md:
##########
@@ -1,6 +1,6 @@
# Scalar Functions Support Status
-**Out of 433 scalar functions in Spark 4.1, Gluten currently fully supports
261 functions and partially supports 31 functions.**
+**Out of 433 scalar functions in Spark 4.1, Gluten currently fully supports
262 functions and partially supports 31 functions.**
Review Comment:
The headline and table are already emitted by the same generator,
tools/scripts/gen-function-support-docs.py: generate_function_doc computes
num_supported/num_partially_supported and writes the summary as well as the
table. This patch follows its existing accounting for the shared
ShiftRightUnsigned entry; it does not introduce a separate counting mechanism.
I have not claimed a fresh full Spark-based document-generation run.
##########
backends-velox/src/test/scala/org/apache/gluten/functions/ScalarFunctionsValidateSuite.scala:
##########
@@ -197,6 +197,62 @@ class ScalarFunctionsValidateSuite extends
FunctionsValidateSuite {
}
}
+ test("shiftrightunsigned") {
Review Comment:
The current test exercises shiftrightunsigned on every supported Spark
profile and additionally checks >>> on Spark 4.0+. The queries and version
guard make that distinction explicit. The test name is unchanged in the current
patch; including the operator in the name would be a readability-only
refinement, not a coverage change.
##########
backends-velox/src/test/scala/org/apache/gluten/functions/ScalarFunctionsValidateSuite.scala:
##########
@@ -197,6 +197,62 @@ class ScalarFunctionsValidateSuite extends
FunctionsValidateSuite {
}
}
+ test("shiftrightunsigned") {
+ withTempPath {
+ path =>
+ withTempView("shift_input") {
+ Seq[(Integer, java.lang.Long, Integer)](
+ (8, 8L, 1),
+ (-1, -1L, 1),
+ (Int.MinValue, Long.MinValue, 1),
+ (Int.MinValue, Long.MinValue, 31),
+ (-1, -1L, 32),
+ (-1, -1L, 33),
+ (-1, -1L, 63),
+ (-1, -1L, 64),
+ (-1, -1L, 65),
+ (-1, -1L, -1),
+ (Int.MinValue, Long.MinValue, Int.MaxValue),
+ (null, null, 1),
+ (1, 1L, null)
+ )
+ .toDF("int_value", "long_value", "shift")
+ .write
+ .parquet(path.getCanonicalPath)
+
+
spark.read.parquet(path.getCanonicalPath).createOrReplaceTempView("shift_input")
+ runQueryAndCompare(
+ """
+ |SELECT
+ | shiftrightunsigned(int_value, shift),
+ | shiftrightunsigned(long_value, shift)
+ |FROM shift_input
+ |""".stripMargin) {
+ df =>
+ checkGlutenPlan[ProjectExecTransformer](df)
+ assert(
+
!getExecutedPlan(df).exists(_.isInstanceOf[ColumnarPartialProjectExec]),
+ "Unsigned shifts must not use partial JVM projection")
+ }
+ if (isSparkVersionGE("4.0")) {
+ runQueryAndCompare(
+ """
+ |SELECT
+ | int_value >>> shift,
+ | long_value >>> shift
+ |FROM shift_input
+ |""".stripMargin) {
Review Comment:
Same point as [the other thread](#discussion_r4479630412); the response
there applies to this occurrence too.
##########
backends-velox/src/test/scala/org/apache/gluten/functions/ScalarFunctionsValidateSuite.scala:
##########
@@ -197,6 +197,62 @@ class ScalarFunctionsValidateSuite extends
FunctionsValidateSuite {
}
}
+ test("shiftrightunsigned") {
+ withTempPath {
+ path =>
+ withTempView("shift_input") {
+ Seq[(Integer, java.lang.Long, Integer)](
+ (8, 8L, 1),
+ (-1, -1L, 1),
+ (Int.MinValue, Long.MinValue, 1),
+ (Int.MinValue, Long.MinValue, 31),
+ (-1, -1L, 32),
+ (-1, -1L, 33),
+ (-1, -1L, 63),
+ (-1, -1L, 64),
+ (-1, -1L, 65),
+ (-1, -1L, -1),
+ (Int.MinValue, Long.MinValue, Int.MaxValue),
+ (null, null, 1),
+ (1, 1L, null)
+ )
+ .toDF("int_value", "long_value", "shift")
+ .write
+ .parquet(path.getCanonicalPath)
+
+
spark.read.parquet(path.getCanonicalPath).createOrReplaceTempView("shift_input")
+ runQueryAndCompare(
+ """
+ |SELECT
+ | shiftrightunsigned(int_value, shift),
+ | shiftrightunsigned(long_value, shift)
+ |FROM shift_input
+ |""".stripMargin) {
+ df =>
+ checkGlutenPlan[ProjectExecTransformer](df)
+ assert(
+
!getExecutedPlan(df).exists(_.isInstanceOf[ColumnarPartialProjectExec]),
+ "Unsigned shifts must not use partial JVM projection")
Review Comment:
For now I am keeping the small checks inline in the two callbacks, so each
query's native-project requirement and partial-projection rejection are visible
together. Both callbacks have the same checks and message. Extracting a helper
would be a readability refactor; no helper extraction is included in this
focused coverage fix.
--
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]
