[SPARK-55910][SQL][TESTS] Merge SQLTestUtilsBase/SQLTestUtils into QueryTestBase/QueryTest#55381
[SPARK-55910][SQL][TESTS] Merge SQLTestUtilsBase/SQLTestUtils into QueryTestBase/QueryTest#55381zhengruifeng wants to merge 19 commits intoapache:masterfrom
Conversation
0983e20 to
b120fb6
Compare
…tBase Move all methods from `SQLTestUtilsBase` into `QueryTestBase`, making `SQLTestUtilsBase` a thin alias that extends `QueryTestBase`. This is the first step toward consolidating the test utility class hierarchy by merging `SQLTestUtils` into `QueryTest`. Co-authored-by: Isaac
…erge Co-authored-by: Isaac
…sBase Use fully qualified name to avoid unused import warning while ensuring the type is resolved correctly from the sub-package. Co-authored-by: Isaac
…Utils Co-authored-by: Isaac
…nTest Co-authored-by: Isaac
Move all methods from `SQLTestUtils` into `QueryTest`, making `SQLTestUtils` a thin alias. Also move `compareAnswers` into `object QueryTest`. Co-authored-by: Isaac
… abstract class SQLTestUtils must remain a trait extending only traits (QueryTestBase, PlanTest) rather than the abstract class QueryTest, because many test suites mix in SQLTestUtils with SparkFunSuite or SparkPlanTest directly. Co-authored-by: Isaac
SQLTestUtils cannot extend QueryTest (abstract class) because many test suites mix SQLTestUtils with SparkFunSuite or SparkPlanTest directly. Methods that call test() must stay in a class/trait that extends SparkFunSuite. Reverting to keep SQLTestUtils unchanged. This PR now only merges SQLTestUtilsBase into QueryTestBase. Co-authored-by: Isaac
…sBase Co-authored-by: Isaac
…for QueryTestBase Co-authored-by: Isaac
…Utils into it Change QueryTest from abstract class to trait, enabling SQLTestUtils to extend it directly. Move all SQLTestUtils methods into QueryTest. Both SQLTestUtils and SQLTestUtilsBase are now thin aliases. Co-authored-by: Isaac
…QLTestUtilsBase Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
…QueryTest When QueryTest was an abstract class, getStackTrace()(2) reliably returned the caller's frame. As a trait with default methods, the stack layout may differ. Use dynamic stack walking to find the first frame outside QueryTest.scala instead of a hardcoded index. Co-authored-by: Isaac
… filename Find the current method's frame by name, then take the next frame as the caller. No hardcoded filenames or indices. Co-authored-by: Isaac
…thods Use lastIndexWhere instead of indexWhere to find the method's frame, skipping any mixin forwarder frames that trait compilation may add. Co-authored-by: Isaac
52fec98 to
df22c24
Compare
cloud-fan
left a comment
There was a problem hiding this comment.
This PR unifies the parallel QueryTestBase/QueryTest and SQLTestUtilsBase/SQLTestUtils hierarchies. QueryTestBase absorbs everything from SQLTestUtilsBase (gaining Eventually, BeforeAndAfterAll, SQLTestData mixins plus ~22 helpers), QueryTest absorbs everything from SQLTestUtils and is converted from abstract class to trait so it can be mixed in alongside SparkFunSuite/SparkPlanTest. SQLTestUtilsBase and SQLTestUtils are left as empty aliases for backward compatibility.
The mechanical move looks clean. The trait conversion is backward compatible — every existing suite uses extends QueryTest with ..., so SparkFunSuite stays anchored in the linearization. The lastIndexWhere stack-trace fix correctly handles both plain trait-default and mixin-forwarder frame layouts. The two unused-import cleanups (AlterTableTests, HivePlanTest) are correct — checkAnswer and sql are now inherited through QueryTestBase.
A few things to address:
- Keeping
SQLTestUtils/SQLTestUtilsBaseas empty aliases for backward compatibility is fine, but their scaladoc should say so explicitly (and steer new code toQueryTest/QueryTestBase), and the useful guidance that was previously onSQLTestUtilsshould move ontoQueryTestrather than getting dropped. - Moving the
DisableAdaptiveExecution-awaretest()override fromSQLTestUtilsontoQueryTestis a behavior expansion that isn't called out in the PR description — see the inline comment. - A couple of small cleanups (redundant import, redundant self-type).
| * | ||
| * Subclasses should *not* create `SparkSession`s in the test suite constructor, which is | ||
| * prone to leaving multiple overlapping [[org.apache.spark.SparkContext]]s in the same JVM. | ||
| * code base. Now a thin alias for [[QueryTest]]. |
There was a problem hiding this comment.
Since the traits are kept as empty aliases for backward compatibility, their scaladoc should explain that explicitly — e.g. "Kept as an empty alias of QueryTest for backward compatibility with existing subclasses. New test suites should extend QueryTest/QueryTestBase directly." Right now "Now a thin alias for [[QueryTest]]" tells a reader about the aliasing but not the intent (backward compat, don't use for new code). Same for SQLTestUtilsBase on line 33.
Separately, the old SQLTestUtils scaladoc carried useful guidance that shouldn't be lost — "Subclasses should not create SparkSessions in the test suite constructor, which is prone to leaving multiple overlapping SparkContexts…" and "import testImplicits._ instead of through the SparkSession." Please lift that onto the QueryTest trait (QueryTest.scala:541) so future readers still see it.
There was a problem hiding this comment.
updated the comments
| .map(_.toFile.length).sum | ||
| } | ||
| } | ||
| private[sql] trait SQLTestUtilsBase extends QueryTestBase { self: Suite => } |
There was a problem hiding this comment.
QueryTestBase already has { self: Suite => }, so the explicit self-type here is redundant.
| private[sql] trait SQLTestUtilsBase extends QueryTestBase { self: Suite => } | |
| private[sql] trait SQLTestUtilsBase extends QueryTestBase |
There was a problem hiding this comment.
it is needed, otherwise
[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/test/SQLTestUtils.scala:33:45: illegal inheritance;
[error] self-type org.apache.spark.sql.test.SQLTestUtilsBase does not conform to org.apache.spark.sql.QueryTestBase's selftype org.apache.spark.sql.QueryTestBase with org.scalatest.Suite
[error] private[sql] trait SQLTestUtilsBase extends QueryTestBase
[error] ^
[warn] two warnings found
[error] one error found
[error] (sql / Test / compileIncremental) Compilation failed
[error] Total time: 452 s (0:07:32.0), completed Apr 20, 2026, 10:26:54 AM
| import org.apache.spark.sql.catalyst.analysis.NoSuchTableException | ||
| import org.apache.spark.sql.catalyst.catalog.SessionCatalog.DEFAULT_DATABASE | ||
| import org.apache.spark.sql.catalyst.plans._ | ||
| import org.apache.spark.sql.catalyst.plans.PlanTest |
There was a problem hiding this comment.
The wildcard import org.apache.spark.sql.catalyst.plans._ on line 41 already brings in PlanTest, so this explicit import is redundant.
| } | ||
| } | ||
|
|
||
| override protected def test(testName: String, testTags: Tag*)(testFun: => Any) |
There was a problem hiding this comment.
This override previously lived on SQLTestUtils, so only suites extending SQLTestUtils/SharedSparkSession honored DisableAdaptiveExecution. Moving it onto QueryTest means every extends QueryTest suite now honors the tag — including ones that previously didn't (HivePlanTest, a handful of connector suites). No live regression today (all current @DisableAdaptiveExecution users already go through SharedSparkSession), but worth mentioning in the PR description since it's a semantic expansion beyond "move methods between traits."
There was a problem hiding this comment.
mentioned in PR desc
- Improve scaladoc for SQLTestUtils/SQLTestUtilsBase to explain backward compat intent - Lift the SparkSession usage guidance onto QueryTest trait - Remove redundant self-type on SQLTestUtilsBase - Remove redundant PlanTest import (already covered by wildcard) Co-authored-by: Isaac
Co-authored-by: Isaac
|
thanks, merged into master |
What changes were proposed in this pull request?
Merge both test utility hierarchies into one:
SQLTestUtilsBasemethods intoQueryTestBaseQueryTestfrom abstract class to trait, move allSQLTestUtilsmethods into itSQLTestUtilsBaseandSQLTestUtilsthin empty aliasesChanges in
QueryTest.scala:QueryTestBase(trait):Eventually,BeforeAndAfterAll,SQLTestDatamixinsSQLTestUtilsBase:testImplicits,sql,sparkContext,withSQLConf,withTable,withView,withTempView,withGlobalTempView,withDatabase,withNamespace,withCache,uncacheTable,withTempDatabase,withCurrentCatalogAndNamespace,withLocale,withSessionVariable,withUserDefinedFunction,activateDatabase,stripSparkFilter,logicalPlanToSparkQuery,makeQualifiedPath,testFile,getLocalDirSizeQueryTest(changed from abstract class to trait):PlanTestmixinSQLTestUtils:setupTestData,loadTestData,beforeAll,withTempDir,testWithWholeStageCodegenOnAndOff,testQuietly,testoverride (DisableAdaptiveExecution),testWithUninterruptibleThread,withResourceTempPath,waitForTasksToFinish,withTempPathsgetCurrentClassCallSitePatternandgetNextLineCallSitePatternto uselastIndexWherefor stack frame lookup — trait default methods may add mixin forwarder frames that shift the stack layoutChanges in
SQLTestUtils.scala:SQLTestUtilsBase→ empty trait alias ofQueryTestBaseSQLTestUtils→ empty trait alias ofQueryTestobject SQLTestUtils.compareAnswerskept as-isUnused import fixes:
AlterTableTests.scala— removed unusedQueryTest.checkAnswerstatic importHivePlanTest.scala— removed unusedimport spark.sql(now inherited)Why are the changes needed?
Consolidates two parallel test utility hierarchies (
QueryTestBase/QueryTestandSQLTestUtilsBase/SQLTestUtils) into one. Previously they sharedPlanTestBaseandSparkSessionProviderbut were maintained as separate hierarchies, causing confusion about which to extend.After this PR:
Changing
QueryTestfrom abstract class to trait enablesSQLTestUtilsto extend it directly, and allows test suites to mix inQueryTestalongsideSparkFunSuiteorSparkPlanTestwithout inheritance conflicts.Semantic change:
DisableAdaptiveExecutiontag handlingThe
test()override that honors@DisableAdaptiveExecutionpreviously lived only onSQLTestUtils. Moving it ontoQueryTestmeans everyextends QueryTestsuite now honors the tag — including ones that previously didn't (e.g.,HivePlanTestand a handful of connector suites that extendQueryTestdirectly without going throughSharedSparkSession). No live regression today since all current@DisableAdaptiveExecutionusages go throughSharedSparkSession, but this is a semantic expansion beyond just moving methods between traits.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-4-6)