sunchao commented on code in PR #7110:
URL: https://github.com/apache/datafusion-comet/pull/7110#discussion_r4315490148
##########
spark/src/test/scala/org/apache/spark/sql/CometTPCDSQueryTestSuite.scala:
##########
@@ -210,7 +210,13 @@ class CometTPCDSQueryTestSuite extends QueryTest with
TPCDSBase with CometSQLQue
allJoinConfCombinations.contains(conf),
s"Join configurations [$conf] should be one of
$allJoinConfCombinations"))
- if (tpcdsDataPath.nonEmpty) {
+ /**
+ * Registers the TPC-DS tests. A subclass that overrides `tpcdsQueries` must
call this at the
+ * end of its own body, once its overrides are initialized. Calling it from
this class's
+ * constructor would see them as null, which previously required an early
initializer in the
+ * subclass.
+ */
+ protected def registerTpcdsQueryTests(): Unit = if (tpcdsDataPath.nonEmpty) {
Review Comment:
[P2] Preserve registration for the concrete parent suite too. Moving this
constructor block into `registerTpcdsQueryTests()` leaves
`CometTPCDSQueryTestSuite` with no caller, although it remains directly
runnable and listed in both CI suite lists. With `SPARK_TPCDS_DATA` pointing to
valid table directories, selecting
`-Dsuites=org.apache.spark.sql.CometTPCDSQueryTestSuite` now silently runs zero
tests instead of registering 130. The child's 134 tests do not fully replace
this coverage: it excludes `q39a` and `q39b`, which the parent registered.
Please retain registration for both concrete entry points, for example through
an abstract shared implementation and explicit registration in each concrete
suite.
Evidence: A constructor-only harness compiled the base parent, exact-head
parent, and exact-head child against Spark 4.1.3 and Scala 2.13.17. With 24
empty table directories satisfying the constructor's path checks, it printed:
`base parent registered=130`, `head parent registered=0`, `head child
registered=134`, and `base-only tests=q39a,q39b`. The harness used fixtures
only for unrelated Comet configuration constants and did not execute SQL. The
data-backed CI job selects only the child, explaining why its 134 passing tests
do not detect this regression.
--
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]
