cloud-fan commented on code in PR #55571:
URL: https://github.com/apache/spark/pull/55571#discussion_r3298222821
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala:
##########
@@ -87,6 +88,30 @@ class DataSourceV2DataFrameSuite
catalog.asInstanceOf[InMemoryTableCatalog]
}
+ // DSv2ExternalMutationTestBase implementations for classic mode
+ override protected def testPrefix: String = "[classic] "
+
+ override protected def withTestSession(fn: SparkSession => Unit): Unit =
fn(spark)
+
+ override protected def checkRows(df: => DataFrame, expected: Seq[Row]): Unit
=
+ checkAnswer(df, expected)
+
+ override protected def getTableCatalog[C <: TableCatalog: ClassTag](
+ session: SparkSession,
+ catalogName: String): C = {
+ catalog(catalogName).asInstanceOf[C]
Review Comment:
The `ClassTag` context bound is declared but unused here —
`catalog(catalogName).asInstanceOf[C]` is a no-op at the boundary under
erasure, so a type mismatch surfaces later as a confusing `ClassCastException`
on a downstream method call. The Connect impl
(`DataSourceV2TempViewConnectSuite.scala:62-66`) already does the right thing
with `require(ct.runtimeClass.isInstance(catalog), ...)`; mirroring it here
keeps the failure mode symmetric.
```suggestion
val c = catalog(catalogName)
val ct = implicitly[ClassTag[C]]
require(
ct.runtimeClass.isInstance(c),
s"Expected ${ct.runtimeClass.getName} but got ${c.getClass.getName}")
c.asInstanceOf[C]
```
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala:
##########
@@ -87,6 +88,30 @@ class DataSourceV2DataFrameSuite
catalog.asInstanceOf[InMemoryTableCatalog]
}
+ // DSv2ExternalMutationTestBase implementations for classic mode
+ override protected def testPrefix: String = "[classic] "
Review Comment:
`${testPrefix}` renames the 21 classic tests from e.g. `"temp view with
stored plan reflects session write"` to `"[classic] temp view with stored plan
reflects session write"`. CI test-result trackers and flaky-test dashboards key
on test name, so this looks like 21 deletions + 21 additions and the existing
history is lost.
The existing Spark precedents for sharing tests between classic and Connect
(`StaticProcedureSuiteBase`,
`sql.SparkSessionBuilderImplementationBindingSuite`) use plain test names and
rely on the suite class name to disambiguate — that would preserve history here
and align with the established convention. If you want the `[connect] ` tag for
readability on the Connect side specifically, you can leave Connect's prefix
as-is and set the classic prefix to `""` so the existing names are preserved.
--
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]
