Copilot commented on code in PR #12981:
URL: https://github.com/apache/gluten/pull/12981#discussion_r3966043894
##########
backends-clickhouse/src/test/scala/org/apache/gluten/execution/GlutenFunctionValidateSuite.scala:
##########
@@ -699,17 +699,10 @@ class GlutenFunctionValidateSuite extends
GlutenClickHouseWholeStageTransformerS
}
test("test common subexpression eliminate") {
+ // TODO: the expected operator counts at the call sites below only held on
Spark 3.3. Re-derive
+ // them for the supported versions and turn this back into a real
assertion.
def checkOperatorCount[T <: TransformSupport](count: Int)(df:
DataFrame)(implicit
- tag: ClassTag[T]): Unit = {
- if (spark33) {
- assert(
- getExecutedPlan(df).count(
- plan => {
- plan.getClass == tag.runtimeClass
- }) == count,
- s"executed plan: ${getExecutedPlan(df)}")
- }
- }
+ tag: ClassTag[T]): Unit = {}
Review Comment:
Same issue as the other `checkOperatorCount` helper: it now performs no
assertions, reducing the effectiveness of these validation tests. Prefer making
the impacted tests explicitly ignored (so CI reports them) or restoring
assertions with updated expected counts for supported Spark versions.
##########
backends-clickhouse/src/test/scala/org/apache/gluten/execution/tpch/GlutenClickHouseTPCHParquetRFSuite.scala:
##########
@@ -55,19 +53,7 @@ class GlutenClickHouseTPCHParquetRFSuite extends
GlutenClickHouseTPCHSaltNullPar
|
|""".stripMargin,
compareResult = true,
- df => {
- if (spark33) {
- val filterExecs = df.queryExecution.executedPlan.collect {
- case filter: FilterExecTransformerBase => filter
- }
- assert(filterExecs.size == 4)
- assert(
- filterExecs.head
- .asInstanceOf[FilterExecTransformer]
- .toString
- .contains("might_contain"))
- }
- }
+ NOOP
Review Comment:
The `NOOP` callback must be in scope to compile. In this hunk the wildcard
import that likely provided it was removed; if `NOOP` is no longer
imported/defined by a base trait, this will cause a compile error. Prefer an
explicit no-op lambda at the call site (e.g., `(_ => ())`) or reintroduce the
specific import/definition that provides `NOOP`.
##########
backends-clickhouse/src/test/scala/org/apache/gluten/execution/hive/GlutenClickHouseHiveTableSuite.scala:
##########
@@ -1038,25 +1038,13 @@ class GlutenClickHouseHiveTableSuite
spark.sql(
s"CREATE FUNCTION my_add as " +
s"'org.apache.hadoop.hive.contrib.udf.example.UDFExampleAdd2' USING
JAR '$jarUrl'")
- if (isSparkVersionLE("3.3")) {
- runQueryAndCompare("select MY_ADD(id, id+1) from range(10)")(
- checkGlutenPlan[ProjectExecTransformer])
- } else {
- runQueryAndCompare("select MY_ADD(id, id+1) from range(10)", noFallBack
= false)(_ => {})
- }
+ runQueryAndCompare("select MY_ADD(id, id+1) from range(10)", noFallBack =
false)(_ => {})
}
+ // TODO: the expected operator counts at the call sites below only held on
Spark 3.3. Re-derive
+ // them for the supported versions and turn this back into a real assertion.
def checkOperatorCount[T <: TransformSupport](count: Int)(df:
DataFrame)(implicit
- tag: ClassTag[T]): Unit = {
- if (spark33) {
- assert(
- getExecutedPlan(df).count(
- plan => {
- plan.getClass == tag.runtimeClass
- }) == count,
- s"executed plan: ${getExecutedPlan(df)}")
- }
- }
+ tag: ClassTag[T]): Unit = {}
Review Comment:
This helper is now a no-op, so any call sites that previously asserted
operator counts no longer validate plan shape at all. If the counts are
currently unknown, consider either (a) converting the specific tests that rely
on this into `ignore` with a tracking issue, or (b) updating the assertion
logic to match the supported Spark versions so the tests continue to provide
coverage.
##########
backends-velox/src/test/scala/org/apache/spark/sql/execution/VeloxParquetWriteForHiveSuite.scala:
##########
@@ -298,7 +291,7 @@ class VeloxParquetWriteForHiveSuite extends GlutenQueryTest
with SQLTestUtils wi
reader =>
val column =
reader.getFooter.getBlocks.get(0).getColumns.get(0)
// native writer and vanilla spark hive writer should be
consistent
- "zstd".equalsIgnoreCase(column.getCodec.toString)
+ assert("zstd".equalsIgnoreCase(column.getCodec.toString))
Review Comment:
The new assertion is good, but it will be hard to debug on failure without
context. Consider adding a failure message that includes the actual codec value
(and possibly file path / writer mode) so CI failures are actionable.
--
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]
