cloud-fan commented on code in PR #56190: URL: https://github.com/apache/spark/pull/56190#discussion_r3456218481
########## sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTestBase.scala: ########## @@ -0,0 +1,60 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql + +// scalastyle:off funsuite +import org.scalatest.funsuite.AnyFunSuite + +import org.apache.spark.CheckErrorHelper +import org.apache.spark.sql.catalyst.SQLConfHelper +import org.apache.spark.sql.internal.SQLConf +// scalastyle:on + +/** + * TODO should be moved to sql/api + * + * base for fully sql/core independent tests, i.e. this trait could be moved to sql/api and then + * used in sql/connect/client. + */ +trait SessionQueryTestBase + extends AnyFunSuite + with SparkSessionProvider + with CheckAnswerHelper + with CheckErrorHelper + with SQLConfHelper Review Comment: I'd add a single agnostic `withConf(key -> value)` backed by `spark.conf` (`RuntimeConfig`). It works in both modes and covers every non-static conf (public *and* internal), since classic and Connect both ultimately call `setConfString` on the session's `SQLConf`. Tests keep referencing the typed key (`withConf(SQLConf.SOME_CONF.key -> "v")`) — that's just reading a constant. I would *not* reimplement `withSQLConf` over `spark.conf` (option 1): overloading the name silently narrows its power (a test setting an internal/thread-local conf would behave differently), which is the kind of silent divergence we want to avoid. Keep `withSQLConf` as a classic-only escape hatch — it's inherently `SQLConf`-typed — alongside the other classic-only helpers. Static confs aren't settable by either `withSQLConf` or `withConf` and stay on the `sparkConf` override (which already reaches the Connect server session, since the in-process server runs on the classic `SparkContext`), so `withConf` loses nothing there. Two impl notes: (1) `withConf` must save+restore the prior value including the *unset* case, or conf state leaks across tests; (2) worth a quick check that Connect's config service doesn't filter `.internal()` keys — if it does, those few tests become classic-only, which is an honest constraint rather than a gap to paper over. ########## sql/core/src/test/scala/org/apache/spark/sql/test/SharedSparkSession.scala: ########## @@ -19,40 +19,13 @@ package org.apache.spark.sql.test import scala.concurrent.duration._ -import org.scalatest.{BeforeAndAfterEach, Suite} -import org.scalatest.concurrent.Eventually +import org.scalatest.Suite -import org.apache.spark.{DebugFilesystem, SparkConf} -import org.apache.spark.internal.config.UNSAFE_EXCEPTION_ON_MEMORY_LEAK -import org.apache.spark.sql.{classic, QueryTest, QueryTestBase, SparkSession, SparkSessionProvider, SQLContext} -import org.apache.spark.sql.catalyst.expressions.CodegenObjectFactoryMode -import org.apache.spark.sql.catalyst.optimizer.ConvertToLocalRelation -import org.apache.spark.sql.internal.{SQLConf, StaticSQLConf} +import org.apache.spark.sql.{QueryTest, QueryTestBase, SparkSessionBinderBase} +import org.apache.spark.sql.classic -trait SharedSparkSession extends QueryTest with SharedSparkSessionBase { - - /** - * Suites extending [[SharedSparkSession]] are sharing resources (e.g. SparkSession) in their - * tests. That trait initializes the spark session in its [[beforeAll()]] implementation before - * the automatic thread snapshot is performed, so the audit code could fail to report threads - * leaked by that shared session. - * - * The behavior is overridden here to take the snapshot before the spark session is initialized. - */ - override protected val enableAutoThreadAudit = false - - protected override def beforeAll(): Unit = { - doThreadPreAudit() - super.beforeAll() - } - - protected override def afterAll(): Unit = { - try { - super.afterAll() - } finally { - doThreadPostAudit() - } - } +@deprecated("Use SessionQueryTest (or classic.SessionQueryTest if required) instead", "4.2.0") Review Comment: I'd decouple advertising from deprecating. Deprecating `SharedSparkSession` now fires a warning across ~500 call sites and points them at a base whose Connect path is still red on this commit (the `CheckAnswerHelper.isDfSorted` throw breaks the binder-swap demo `QueryTestWithConnectSuite`), with UX gaps still open (the config helper above, ordering on Connect). That's a lot of churn toward a moving target. So: land the new traits now but **drop the `@deprecated` on `SharedSparkSession` for now**, and advertise `SessionQueryTest` via the AGENTS.md / test-style-doc update — new suites get steered to the new way immediately without deprecating anything. Flip `@deprecated` in a follow-up once we've dogfooded and closed the gaps. The gating signal is 'gaps closed + docs landed', not a fixed month — which also matches your instinct to find the UX gaps ourselves first. -- 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]
