Copilot commented on code in PR #58115:
URL: https://github.com/apache/spark/pull/58115#discussion_r3812021682
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/WholeStageCodegenSuite.scala:
##########
@@ -35,6 +35,10 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.test.SharedSparkSession
import org.apache.spark.sql.types.{DayTimeIntervalType, DecimalType,
DoubleType, FloatType, IntegerType, LongType, StringType, StructField,
StructType}
+case class Spark51356Inner(d: Int)
+case class Spark51356Mid(c: Spark51356Inner = null)
+case class Spark51356Outer(b: Spark51356Mid = null)
Review Comment:
These case classes are introduced at the package level in a shared test
source file, which increases the chance of namespace pollution and accidental
reuse/collision across test suites in the same package. Consider nesting them
inside `WholeStageCodegenSuite` (or a companion object) and marking them
`private` to keep the scope limited to this test.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/WholeStageCodegenSuite.scala:
##########
@@ -1617,4 +1621,29 @@ class WholeStageCodegenSuite extends SharedSparkSession
assert(sinhPattern.findAllIn(inlineCode).length == 1,
"sinh(v) should be evaluated only once per input row without function
splitting")
}
+
+ test("SPARK-51356: FilterExec incorrectly reorders IsNotNull predicates for
nested access") {
+ import testImplicits._
+ val data = Seq(
+ Spark51356Outer(null),
+ Spark51356Outer(Spark51356Mid(null)),
+ Spark51356Outer(Spark51356Mid(Spark51356Inner(0))),
+ Spark51356Outer(Spark51356Mid(Spark51356Inner(1))))
+ val isDZero = udf((c: Spark51356Inner) => c.d == 0)
+
+ val mids = spark.createDataset(data).map(identity)
+ .where(col("b").isNotNull).select(col("b").as[Spark51356Mid])
+ val df = mids.filter(col("c").isNotNull).filter(not(isDZero(col("c"))))
Review Comment:
The regression description focuses on `IsNotNull` over nested/complex
expressions (e.g., `IsNotNull(GetStructField(b, \"c\"))`) being reordered
incorrectly. As written, the test projects `b` into a `Dataset[Spark51356Mid]`
and then filters on `col(\"c\")`, which is a top-level attribute in `mids`;
this may not reliably exercise the nested `GetStructField(b, ...)` `IsNotNull`
pattern implicated in the bug. Consider rewriting the test to keep the nested
access in the filter/UDF input (e.g., filtering and calling the UDF on
`col(\"b.c\")` (and/or `col(\"b.c.d\")`) in the same combined predicate), so
the plan contains an `IsNotNull` on a nested expression and validates the
corrected ordering in whole-stage codegen.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/WholeStageCodegenSuite.scala:
##########
@@ -1617,4 +1621,29 @@ class WholeStageCodegenSuite extends SharedSparkSession
assert(sinhPattern.findAllIn(inlineCode).length == 1,
"sinh(v) should be evaluated only once per input row without function
splitting")
}
+
+ test("SPARK-51356: FilterExec incorrectly reorders IsNotNull predicates for
nested access") {
+ import testImplicits._
+ val data = Seq(
+ Spark51356Outer(null),
+ Spark51356Outer(Spark51356Mid(null)),
+ Spark51356Outer(Spark51356Mid(Spark51356Inner(0))),
+ Spark51356Outer(Spark51356Mid(Spark51356Inner(1))))
+ val isDZero = udf((c: Spark51356Inner) => c.d == 0)
+
+ val mids = spark.createDataset(data).map(identity)
Review Comment:
`map(identity)` is a no-op transformation that still adds an extra Dataset
operator and codegen work to the test plan, which can make the regression test
slower and the executed plan less focused on the behavior under test. Dropping
it should keep the test intent the same while reducing plan noise.
--
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]
