weiting-chen commented on code in PR #13163: URL: https://github.com/apache/gluten/pull/13163#discussion_r4226533636
########## backends-velox/src/test/scala/org/apache/gluten/execution/MiscOperatorSuite.scala: ########## @@ -37,6 +37,11 @@ import java.util.concurrent.TimeUnit import scala.collection.JavaConverters import scala.collection.JavaConverters._ +// GLUTEN-12569 Spark 4.2 UT enablement: disabled whole suite - beforeAll registers the +// 1-part FunctionIdentifier "velox_dummy_expression" (VeloxDummyExpression.registerFunctions), +// which Spark 4.2's FunctionRegistry rejects (must be fully-qualified 3-part), aborting the +// entire group1 CI run. Disabled here so the rest of the group can run and be triaged. [email protected] Review Comment: **Scope the shared-suite ignores to Spark 4.2** **Target Location:** `backends-velox/src/test/scala/org/apache/gluten/execution/MiscOperatorSuite.scala:44` and `backends-velox/src/test/scala/org/apache/gluten/sql/GlutenBloomFilterFallbackSuite.scala:55`. **Problem:** Both new class-level `@Ignore` annotations are on shared backend suites, not Spark42-only sources. They therefore suppress regression coverage on Spark 3.4/3.5/4.0/4.1 as well. Deferring the Spark42 FunctionRegistry incompatibility in #13179 is reasonable, but the mitigation should not also disable these suites for already-supported versions. This is a high-confidence test-gate regression, not a claim of a new production wrong-result bug. **Evidence:** ```scala @org.scalatest.Ignore class MiscOperatorSuite extends VeloxWholeStageTransformerSuite with AdaptiveSparkPlanHelper { ``` The same unconditional annotation precedes `GlutenBloomFilterFallbackSuite`. Existing group1 jobs select both via `org.apache.gluten` in the shared `backends-velox` reactor. The current [Spark35 group1 log](https://github.com/apache/gluten/actions/runs/37813514433/job/113439077021) reports all seven bloom-filter tests ignored (including Spark-readable bloom bytes and whole-stage fallback/reversion) and 99 MiscOperator tests ignored. One MiscOperator case was already explicitly ignored, so **105 previously registered nonignored tests are newly suppressed**. This is visible in the green CI run itself; it is not inferred solely from the annotation. **Suggested Fix:** Remove the class-level annotation from **both** shared suites and apply an early, Spark42-only ignore instead. For example, the existing version helper and a conditional tag override preserve the older-version tests and their original tags: ```scala override def tags: Map[String, Set[String]] = { val inherited = super.tags if (matchSparkVersion(Some("4.2"), Some("4.2"))) { inherited ++ testNames.map { name => name -> (inherited.getOrElse(name, Set.empty[String]) + "org.scalatest.Ignore") }.toMap } else { inherited } } ``` Keep the #13179 explanation next to the version-scoped skip. In ScalaTest 3.2.16, ignoring all registered tests makes the expected runnable count zero and prevents these suites' `beforeAll` setup from running; guarding only the test bodies would be too late. The snippet is source-verified, not runtime-tested. Please verify both complete suites actually execute on Spark35/41 and remain intentionally ignored without setup aborts on Spark42. -- 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]
