Copilot commented on code in PR #13214:
URL: https://github.com/apache/gluten/pull/13214#discussion_r4479970808
##########
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) {
Review Comment:
These queries don’t specify an ORDER BY. Reads from Parquet can be
non-deterministic in row order, which can make parity comparisons flaky
depending on how `runQueryAndCompare` validates results. Adding a deterministic
ORDER BY (e.g., on `int_value`, `long_value`, `shift`) would make the test more
stable.
##########
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:
These queries don’t specify an ORDER BY. Reads from Parquet can be
non-deterministic in row order, which can make parity comparisons flaky
depending on how `runQueryAndCompare` validates results. Adding a deterministic
ORDER BY (e.g., on `int_value`, `long_value`, `shift`) would make the test more
stable.
##########
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:
This supported-function count is hardcoded and can easily drift from the
table contents over time. Consider generating the counts from the table (or
removing the exact counts) so the documentation stays consistent without manual
updates.
##########
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) {
+ df =>
+ checkGlutenPlan[ProjectExecTransformer](df)
+ assert(
+
!getExecutedPlan(df).exists(_.isInstanceOf[ColumnarPartialProjectExec]),
+ "Unsigned shifts must not use partial JVM projection")
Review Comment:
The plan assertions are duplicated across the function and operator
branches. Consider extracting a small local helper (e.g.,
`assertNativeUnsignedShift(df)`) to avoid duplication and make future
adjustments (like changing the plan-shape criteria) easier and less error-prone.
##########
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:
The plan assertions are duplicated across the function and operator
branches. Consider extracting a small local helper (e.g.,
`assertNativeUnsignedShift(df)`) to avoid duplication and make future
adjustments (like changing the plan-shape criteria) easier and less error-prone.
--
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]
