You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "fwc (via GitHub)" <gi...@apache.org> on 2026/05/28 20:17:35 UTC
[PR] [SPARK-56505][SQL][TESTS] Add SparkSessionBinder to replace SharedSparkSession [spark]
fwc opened a new pull request, #56190:
URL: https://github.com/apache/spark/pull/56190
### What changes were proposed in this pull request?
- Introduces `sql.SparkSessionBinder` and `classic.SparkSessionBinder` as 'implementor' counterparts to the 'declarators' `sql.SparkSessionProvider` and `classic.SparkSessionProvider`.
- Deprecates `SharedSparkSession` with the hint that `SparkSessionBinder` and `QueryTest` shall be used instead.
### Why are the changes needed?
Currently, most tests use `SharedSparkSession` to obtain the `spark` object. This effectively prevents specializing these tests in `sql/connect` as `SharedSparkSession` provides a `classic.SparkSession`, thus preventing overriding.
This PR deprecates `SharedSparkSession` and instead introduces `sql.SparkSessionBinder` and `classic.SparkSessionBinder`. If some `FooSuite` now uses the `sql.SparkSessionBinder` trait like e.g.
```
class FooSuite extends SparkSessionBinder with QueryTest {
checkAnswer(
sql("SELECT 1"),
Seq(1)
)
}
```
We can now add a connect variant of that suite as follows:
```
class FooWithConnectSuite extends FooSuite
with connect.SparkSessionBinder
with connect.QueryTest
```
### Does this PR introduce _any_ user-facing change?
This PR extends the `beforeAll`/`afterAll` of `SharedSparkSessionBase` to include the the thread audit check, which was previously only present in `SharedSparkSession`.
AFAICS, `SparkSessionBase` is neither used in delta lake nor in apache iceberg.
### How was this patch tested?
This patch is test-only.
### Was this patch authored or co-authored using generative AI tooling?
Parts of this patch were authored by claude code
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3454214218
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTest.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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
+
+import org.apache.spark.SparkFunSuite
+
+/**
+ * Provides connect-compatible test utils to write suites that have 'connect variants':
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
Review Comment:
Err yeah the docstring is wrong, should be `FooConnectSuite extends FooSuite with connect.SessionQueryTest` 😅
So `protected` utils can just be re-used.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SparkSessionBinder to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3359763783
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -1211,7 +1211,7 @@ object QueryTest extends Assertions {
}
-class QueryTestSuite extends test.SharedSparkSession {
+class QueryTestSuite extends QueryTest with SparkSessionBinder {
Review Comment:
For scala tests in `sql/core`, I envision a test style (and corresponding infra) that encourages:
1. Testing at the [Spark SQL/DataFrame API](https://spark.apache.org/docs/latest/api/scala/org/apache/spark/sql/index.html) level (i.e. discourages/prevents accessing internals)
1. Targeting both classic _and connect_
1. A rather [DAMP than DRY](https://abseil.io/resources/swe-book/html/ch12.html#tests_and_code_sharing_dampcomma_not_dr) style (i.e. tests as `spark` and `df` ops + checks with little/no 'abstraction')
While _DAMP, not DRY_ would mostly be enshrined in guidelines like `AGENTS.md`, a testing style guide, or a `sql/core/src/test/README.md`, I think that _testing the API using classic and connect_ can be made easier by further tweaking and extending the testing APIs / traits.
In some sense, this 'proposal' isn't proposing something 'new': Many (most?) suites are already more _DAMP_ than _DRY_ and with the introduction of `sql.SparkSessionProvider` and its usage in `QueryTest`, writing classic/connect-agnostic tests is already much easier than before. But (a) afaics _DAMP NOT DRY_ is not (yet) codified anywhere and (b) I think the testing traits can be further improved to make 'doing the right thing' even 'more easier'.
So, I propose the following:
- __Introduce a fully classic/connect-agnostic `SparkSessionTest` as the default SQL/DataFrame API testing trait__
- __Encourage the addition of 'connect variants'__ by e.g. adding a linter that warns/fails if suite `X` implements `sql.SparkSessionTest` but there is no corresponding `class Y extends X with connect.SparkSessionTest` or (if possible), automagically generating the connect variant.
- __Discourage access to internals for newly added suites__ by e.g. adding them in subpackage of `o.a.s.sql.test` or maybe even in a separate compilation unit that only has access to the scala API and test helpers.
## A prototypical Example
This example aims to demonstrate how a suite that adheres to this style and uses `SparkSessionTest` could look like.
This example is inspired by https://github.com/apache/spark/pull/55571.
### The main suite
A prototypical suite extends `SparkSessionTest`, which provides `spark` and useful helpers like `checkAnswer`.
The class defines both the necessary setup (e.g. creating tables, setting confs) and corresponding test cases. The test cases generally consist of Spark SQL/DataFrame operations and assertions (either literal `assert`s or e.g. `checkAnswer`).
The example suite is _not_ located in the same package as the code under test to discourage accessing internals. In this example, internals are accessed using a helper object from the package of the code under test.
```scala
// located in a different package than the code that is tested
package org.apache.spark.sql.test.tablecache
import org.apache.spark.sql.tablecache.TableCacheHelper
// ...
class TableCacheSuite extends SparkSessionTest {
override def beforeAll = {
spark.sql(s"CREATE TABLE $testTable (id INT, salary INT) USING mockedDSv2")
spark.sql(s"INSERT INTO $testTable VALUES (1, 100), (10, 1000)")
}
override def afterAll = {
spark.sql(s"DROP TABLE $testTable")
}
override def sparkConf: SparkConf =
super.sparkConf.set( /* set necessary confs */ )
// the test mostly consists of DataFrame operations,
// util calls like `checkAnswer`, and asserts
test("SPARK-54022: cached table pinned against external data write") {
spark.table(testTable).cache()
assert(spark.catalog.isCached(testTable))
checkAnswer(spark.table(testTable), Seq(Row(1, 100)))
// object that accesses internals
TableCacheHelper.externalAppend(testTable, Row(2, 200))
checkAnswer(spark.table(testTable), Seq(Row(1, 100)))
spark.sql(s"REFRESH TABLE $testTable")
checkAnswer(spark.table(testTable), Seq(Row(1, 100), Row(2, 200)))
assert(spark.catalog.isCached(testTable))
}
test("connector w/ cache: temp view stale after external column removal") {
withView("v") {
spark.table(testTable).filter("salary < 999").createOrReplaceTempView("v")
checkAnswer(spark.table("v"), Seq(Row(1, 100)))
TableCacheHelper.externalDropCol(testTable, "salary")
checkAnswer(spark.table("v"), Seq(Row(1, 100)))
spark.sql(s"REFRESH TABLE $testTable").collect()
checkError(
exception = intercept[AnalysisException] { spark.table("v").collect() },
condition = "INCOMPATIBLE_COLUMN_CHANGES_AFTER_VIEW_WITH_PLAN_CREATION",
)
}
}
}
```
### The 'connect variant'
Besides the main suite, there is a 'connect variant', which runs the same test via Spark Connect:
```scala
package org.apache.spark.sql.connect.test.tablecache
class TableCacheConnectSuite extends TableCacheSuite with SparkSessionTest
```
Ideally, this variant would be auto-generated, so that classic/connect-agnostic testing is the default (i.e. opt-out), rather than something that the developer actively has to strive for.
### The `SparkSessionTest` helper trait
The `SparkSessionTest` used would be the default util trait for SparkSession/DataFrame-level tests, providing a `sql.SparkSession` and utils like `checkAnswer` and `withTable`.
The `SparkSessionTest` trait is designed to be 'classic/connect-agnostic': it only provides utils/APIs that can be used/sensibly overriden in Spark Connect, so that we can have a `connect.SparkSessionTest` sibling trait to facilitate the implementation of the 'connect variant'.
```scala
package org.apache.spark.sql
trait SparkSessionTest extends SparkFunSuite {
def spark: SparkSession = ...
def sql: ...
def checkAnswer(df: DataFrame, exp: Seq[Row]) = ...
def checkError(...) = ...
def withTable(...) = ...
def withView(...) = ...
def withTempDir(...) = ...
// TODO can checks on the plan be classic/connect-agnostic?
}
```
This trait aims to supercede `SharedSparkSession` and `QueryTest`. They shall be deprecated with the suggestion to migrate towards `sql.SparkSessionTest`.
A `classic.SparkSessionTest` can also be added for classic-only tests.
## What's wrong with `SharedSparkSession` and `QueryTest`?
I want to deprecate `SharedSparkSession` because
- It cannot be overriden with a `connect.SharedSparkSession` so classic/connect-agnostic tests would require that testcases are declared in traits/abstract classes.
- It provides a `classic.SparkSession` while currently being the most widely used test trait and the easiest thing to reach for. Changing it would break downstream (e.g. tests in Delta Lake), so I think its usage needs to be actively discouraged.
- It is widely used so the deprecation note will be widely read and can be used to advertise the 'new way'.
I want to deprecate `QueryTest` because
- it is not fully connect/classic-agnostic.
- it provides too many peculiar/specific/rarely used methods.
- it provides implicits.
## On defaults, simplicity, and friction
I propose `SparkSessionTest` as _the default testing trait_ to simplify things a bit: smaller than `QueryTest` but still a one-stop-shop.
Adding a new Suite should be as simple as possible: `class X extends SparkSessionTest`, some setup and _damp_ testcases. No need to declare testcases in an abstract class or trait.
## Structuring test code with inheritance: _variant_ extends _suite_ extends _base_
I suggest the following hierarchy for test classes/traits:
1. __Base__: provides generic utils for its suites, contains no testcases (here: `SparkSessionTest`)
2. __Suite__: contains test cases (here: `TableCacheSuite`)
3. __Variants__: contains overrides (here: `TableCacheConnectSuite`, which overrides `spark`)
While this proposal focuses on 'connect variants', I think the idea can be generalized to e.g. cover different configuration values like e.g. 'codegen on/off'.
---
<details><summary>Appendix: patterns that this proposal discourages</summary>
<p>
The following are exaggerations/caricatures of existing patterns that I believe
to be 'suboptimal'. This proposal aims to discourage such patterns.
### Too much `classic` API usage
`SharedSparkSession` is by far the most widely used test trait (extended/mixed
in ~500 times, while `QueryTest` is used only ~150 times).
The problem: it provides a `classic.SparkSession`, which makes
'classic/connect-agnostic' testing difficult in two ways:
First, `def spark` cannot be overriden to use connect.
Second, usage of classic-only (read: connect-incompatible) APIs is not
discouraged.
```
class FooSuite extends SharedSparkSession {
test("...") {
// classic-only, should use `spark.catalog.setCurrentDatabase(db)`
spark.sessionState.catalogManager.setCurrentNamespace(Array(db))
}
}
```
With this proposal, `SparkSessionTest` hides the used `classic.SparkSession`
behind the `sql` interface so accessing `spark.sessionState.catalogManager`
would give a "Cannot resolve symbol catalogmanager" error in the IDE.
### DRY to the bone
Generally, I do not like concisely abstracted tests like the caricature:
```
abstract class InsertSuite extends QueryTest with InsertMetricCheck {
// creates table t in beforeEach, deletes in afterEach
test("insert 3 rows") {
val data = Seq((1L, "a"), (2L, "b"), (3L, "c"))
doInsert(t, data)
checkInsertMetrics(t, numInsertedRows = 3)
verifyTable(t, data.toDf())
}
}
// potentially in another file:
class InsertSQLSuite extends InsertSuite {
def doInsert(t, d) = sql(s"INSERT INTO $t VALUES ${d.mkString}")
}
class InsertDfwSuite extends InsertSuite {
def doInsert(t, d) = d.toDf().insert.write.insertInto(t)
}
```
I find such testcases unnecessarily hard to read: the logic is scattered over
across functions and files.
While I see the appeal in abstracting over the SQL- and DataFrame-APIs, I think
tests should not hide the concrete SQL- and DataFrame-operations.
(c.f. [Tests and Code Sharing: Damp, Not Dry](https://abseil.io/resources/swe-book/html/ch12.html#tests_and_code_sharing_dampcomma_not_dr))
So I think the strawman should be refactored to the following:
```
class InsertSuite extends SparkSessionTest {
test("insert 3 rows via SQL") {
withTable("foo") {
spark.sql("CREATE TABLE foo (id INT, name STRING)")
spark.sql("INSERT INTO foo VALUES (1, 'a'), (2, 'b'), (3, 'c')")
TableInternalsHelper.getCommits("foo").last match {
case Insert(numRowsInserted) => assert(numRowsInserted === 3)
case c => fail(s"expected Insert commit after inserting, but got $c")
}
checkAnswer(
spark.table("foo"),
Seq(Row(1, "a"), Row(2, "b"), Row(3, "c")),
)
}
}
test("insert 3 rows via dataframe writer API") {
withTable("foo") {
spark.sql("CREATE TABLE foo (id INT, name STRING)")
spark.createDataFrame(Seq(Row(1, "a"), Row(2, "b"), Row(3, "c")))
.write
.insertInto("foo")
TableInternalsHelper.getCommits("foo").last match {
case Insert(numRowsInserted) => assert(numRowsInserted === 3)
case c => fail(s"expected Insert commit after inserting, but got $c")
}
checkAnswer(
spark.table("foo"),
Seq(Row(1, "a"), Row(2, "b"), Row(3, "c")),
)
}
}
}
```
### Long inheritance chains / convoluted matrix tests
When Inspecting the Type Hierarchy of e.g. SharedSparkSession, there are quite
a few _deep_ inheritance chains.
The following shows the chain of `DeltaBasedMergeIntoTableSuiteBase`. The `...Base` classes are abstract.
```
RowLevelOperationSuiteBase 0 cases setup and helpers
.MergeIntoTableSuiteBase 71 cases
..DeltaBasedMergeIntoTableSuiteBase +7 cases overrides expected metrics
...DeltaBasedMergeIntoTableSuite +8 cases sets 'supports-deltas'
...DeltaBasedMergeIntoTableUpdeAsDelete... +1 case sets 'supports-deltas', 'split-updates'
....DeltaBasedMergeIntoTableNoCodegenSuite +0 cases disables codegen via sparkConf
```
As an ideal, I think we should strive for 3 levels (Base, Suite, Variants), like e.g.:
```
RowLevelOperationSuiteBase
.MergeIntoTableSuite
..MergeIntoTableUpdeAsDeleteAndInsertSuite
..MergeIntoTableNoCodegenSuite
..MergeIntoTableDeltaBasedSuite
..MergeIntoTableDeltaBasedUpdeAsDeleteA...
..MergeIntoTableDeltaBasedNoCodegenSuite
```
### Awkward classic/connect-agnostic tests
Some tests introdoce new helper constructs to achieve classic/connect-agnosticity.
```
test("foo") {
withTestSession { session =>
session.sql(s"CREATE TABLE $testTable (id INT, salary INT) USING foo").collect()
session.sql(s"INSERT INTO $testTable VALUES (1, 100)").collect()
checkRows(session.sql(s"SELECT * FROM $testTable"), Seq(Row(1, 100)))
checkRows(session.sql(s"SELECT * FROM $testTable"), Seq(Row(1, 100), Row(2, 200)))
}
}
```
[This diff](https://github.com/apache/spark/pull/56190/changes/6b5dba77015378c074f5bfbd6c26304a1675abff..6f509d8a2c5dfaeb60df8e8485a3e1e7083efc24) shows that these new helpers are not necessary.
</p>
</details>
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SparkSessionBinder to replace SharedSparkSession [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3329781636
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -1211,7 +1211,7 @@ object QueryTest extends Assertions {
}
-class QueryTestSuite extends test.SharedSparkSession {
+class QueryTestSuite extends QueryTest with SparkSessionBinder {
Review Comment:
And `AGENTS.md` already have a section about how to add test suites, we need to update it to whatever final approach people agree on.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3463232589
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/SessionQueryTest.scala:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.connect
+
+import scala.util.matching.Regex
+
+import org.apache.spark.sql
+
+/**
+ * Overrides test utils to implement 'connect variants' of suites declared in sql/core:
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
+ *
+ * // in sql/connect
+ * FooConnectSuite extends FooSuite with connect.SessionQueryTest
+ * }}}
+ *
+ * This trait overrides [[spark]] to use a [[SparkSession connect.SparkSession]], which executes
+ * via the gRPC API using an in-process connect server.
+ */
+trait SessionQueryTest extends sql.SessionQueryTest with SparkSessionBinder {
+
+ private val sortOperator: Regex = "\b(?:Photon)?Sort\b".r
Review Comment:
This regex never matches anything, so `isDfSorted` is always `false` on Connect — every `checkAnswer` silently becomes order-insensitive (the round-4 weakening, back), and `QueryTestWithConnectSuite` will fail the inherited "demands correct result order" test in CI. Two root causes, both need fixing:
1. In a non-raw string literal `\b` is the backspace char (U+0008), not the regex word-boundary — make the literal raw (below).
2. `Regex.matches` matches the *whole* input — the call on line 44 needs a substring match.
Verified in a REPL: a raw `\bSort\b` with `unanchored.matches` detects `Sort` and rejects `SortMergeJoin`/`SortAggregate`, so this also closes last round's over-match concern.
```suggestion
private val sortOperator: Regex = """\b(?:Photon)?Sort\b""".r
```
Line 44 also needs `.unanchored` — see the next comment.
##########
sql/core/src/test/scala/org/apache/spark/sql/CheckAnswerHelper.scala:
##########
@@ -0,0 +1,215 @@
+/*
+ * 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
+
+import java.util.TimeZone
+
+import org.scalatest.Assertions
+
+import org.apache.spark.annotation.Experimental
+import org.apache.spark.sql.catalyst.ExtendedAnalysisException
+import org.apache.spark.sql.catalyst.plans.logical
+import org.apache.spark.util.{SparkErrorUtils, SparkStringUtils}
+
+/**
+ * Provides [[checkAnswer]] helper for SQL- & DataFrame-API tests.
+ *
+ * TODO: should be moved to sql/api together with SessionQueryTestBase
+ */
+@Experimental
+trait CheckAnswerHelper extends Assertions {
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ protected def checkAnswer(df: => DataFrame, expectedAnswer: Seq[Row]): Unit = {
+
+ val analyzedDF = try df catch {
+ case ae: ExtendedAnalysisException =>
+ if (ae.plan.isDefined) {
+ fail(
+ s"""
+ |Failed to analyze query: $ae
+ |${ae.plan.get}
+ |
+ |${SparkErrorUtils.stackTraceToString(ae)}
+ |""".stripMargin)
+ } else {
+ throw ae
+ }
+ }
+
+ getErrorMessageInCheckAnswer(analyzedDF, expectedAnswer) match {
+ case Some(errorMessage) => fail(errorMessage)
+ case None =>
+ }
+ }
+
+ /*
+ * Note: when moving this to sql/api, implementation should stay in sql/core
+ * (i.e. only have abstract decl in sql/api)
+ */
+ protected def isDfSorted(df: DataFrame): Boolean = {
+ df match {
+ case df: classic.DataFrame =>
+ df.logicalPlan.collectFirst { case s: logical.Sort => s }.nonEmpty
+ case _ =>
+ // isDfSorted should be overriden by connect so that this case can't be reached.
Review Comment:
```suggestion
// isDfSorted should be overridden by connect so that this case can't be reached.
```
##########
sql/connect/common/src/main/scala/org/apache/spark/sql/connect/Dataset.scala:
##########
@@ -241,6 +241,29 @@ class Dataset[T] private[sql] (
// scalastyle:on println
}
+ private[connect] def explainString(mode: String): String = {
+ val protoMode = mode.trim.toLowerCase(util.Locale.ROOT) match {
+ case "simple" => proto.AnalyzePlanRequest.Explain.ExplainMode.EXPLAIN_MODE_SIMPLE
+ case "extended" => proto.AnalyzePlanRequest.Explain.ExplainMode.EXPLAIN_MODE_EXTENDED
+ case "codegen" => proto.AnalyzePlanRequest.Explain.ExplainMode.EXPLAIN_MODE_CODEGEN
+ case "cost" => proto.AnalyzePlanRequest.Explain.ExplainMode.EXPLAIN_MODE_COST
+ case "formatted" => proto.AnalyzePlanRequest.Explain.ExplainMode.EXPLAIN_MODE_FORMATTED
+ case _ => throw new IllegalArgumentException("Unsupported explain mode: " + mode)
+ }
+ sparkSession
+ .analyze(plan, proto.AnalyzePlanRequest.AnalyzeCase.EXPLAIN, Some(protoMode))
+ .getExplain
+ .getExplainString
+ }
+
+ private[connect] def explainString(extended: Boolean): String = if (extended) {
+ explainString("extended")
+ } else {
+ explainString("simple")
+ }
+
+ private[connect] def explainString(): String = explainString("simple")
Review Comment:
This no-arg overload has no callers — only the `Boolean` overload is used (by `connect.SessionQueryTest`). Worth dropping to keep the seam minimal.
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/SessionQueryTest.scala:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.connect
+
+import scala.util.matching.Regex
+
+import org.apache.spark.sql
+
+/**
+ * Overrides test utils to implement 'connect variants' of suites declared in sql/core:
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
+ *
+ * // in sql/connect
+ * FooConnectSuite extends FooSuite with connect.SessionQueryTest
+ * }}}
+ *
+ * This trait overrides [[spark]] to use a [[SparkSession connect.SparkSession]], which executes
+ * via the gRPC API using an in-process connect server.
+ */
+trait SessionQueryTest extends sql.SessionQueryTest with SparkSessionBinder {
+
+ private val sortOperator: Regex = "\b(?:Photon)?Sort\b".r
+
+ /**
+ * Approximates [[sql.SessionQueryTest.isDfSorted]] by inspecting the explain string.
+ */
+ override def isDfSorted(df: sql.DataFrame): Boolean = df match {
+ case df: DataFrame => sortOperator.matches(df.explainString(extended = false))
Review Comment:
`Regex.matches` matches the entire string, so this is `false` for any multi-line explain output. `unanchored` makes `matches` behave like `find` (substring):
```suggestion
case df: DataFrame => sortOperator.unanchored.matches(df.explainString(extended = false))
```
Needs the raw-string fix on line 38 to actually match.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "ionagamed (via GitHub)" <gi...@apache.org>.
ionagamed commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3455575302
##########
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:
AFAICS, even while using the connect path `withSQLConf` behaves the same way as always (at least in spirit).
Do you suggest `withRuntimeConf` to have the same shape and behavior as `withSQLConf`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "ionagamed (via GitHub)" <gi...@apache.org>.
ionagamed commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3491872577
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/ExampleConnectSuite.scala:
##########
@@ -0,0 +1,22 @@
+/*
+ * 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.connect
+
+import org.apache.spark.sql
+
+class ExampleConnectSuite extends sql.ExampleSuite with SessionQueryTest
Review Comment:
Refactored into a separate suite.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "ionagamed (via GitHub)" <gi...@apache.org>.
ionagamed commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3491333926
##########
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 go as far as to say that we shouldn't modify AGENTS.md yet. These changes are experimental, modifying AGENTS.md right away might surface some issues that affect a wide range of usecases and annoy other developers too much.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3453819305
##########
sql/core/src/test/scala/org/apache/spark/sql/CheckAnswerHelper.scala:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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
+
+import java.util.TimeZone
+
+import org.scalatest.Assertions
+
+import org.apache.spark.annotation.Experimental
+import org.apache.spark.sql.catalyst.plans.logical
+import org.apache.spark.util.{SparkErrorUtils, SparkStringUtils}
+
+/**
+ * Provides [[checkAnswer]] helper for SQL- & DataFrame-API tests.
+ *
+ * TODO: should be moved to sql/api together with SessionQueryTestBase
+ */
+@Experimental
+trait CheckAnswerHelper extends Assertions {
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ protected def checkAnswer(df: => DataFrame, expectedAnswer: Seq[Row]): Unit = {
+ getErrorMessageInCheckAnswer(df, expectedAnswer) match {
+ case Some(errorMessage) => fail(errorMessage)
+ case None =>
+ }
+ }
+
+ /*
+ * Note: when moving this to sql/api, implementation should stay in sql/core
+ * (i.e. only have abstract decl in sql/api)
+ */
+ protected def isDfSorted(df: DataFrame): Boolean = {
+ df match {
+ case df: classic.DataFrame =>
+ df.logicalPlan.collectFirst { case s: logical.Sort => s }.nonEmpty
+ case _ => throw new RuntimeException(s"Cannot determine whether df is sorted: $df")
+ }
+ }
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ * If there was exception during the execution or the contents of the DataFrame does not
+ * match the expected result, an error message will be returned. Otherwise, a None will
+ * be returned.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ private def getErrorMessageInCheckAnswer(
+ df: DataFrame,
+ expectedAnswer: Seq[Row]): Option[String] = {
+ val sparkAnswer = try df.collect().toSeq catch {
+ case e: Exception =>
+ val errorMessage =
+ s"""
+ |Exception thrown while executing query:
+ |${df.queryExecution}
+ |== Exception ==
+ |$e
+ |${SparkErrorUtils.stackTraceToString(e)}
+ """.stripMargin
+ return Some(errorMessage)
+ }
+
+ sameRows(expectedAnswer, sparkAnswer, isDfSorted(df)).map { results =>
+ s"""
+ |Results do not match for query:
+ |Timezone: ${TimeZone.getDefault}
+ |Timezone Env: ${sys.env.getOrElse("TZ", "")}
+ |
+ |${df.queryExecution}
+ |== Results ==
+ |$results
+ """.stripMargin
+ }
+ }
+
+ private def prepareAnswer(answer: Seq[Row], isSorted: Boolean): Seq[Row] = {
+ // Converts data to types that we can do equality comparison using Scala collections.
+ // For BigDecimal type, the Scala type has a better definition of equality test (similar to
+ // Java's java.math.BigDecimal.compareTo).
+ // For binary arrays, we convert it to Seq to avoid of calling java.util.Arrays.equals for
+ // equality test.
+ val converted: Seq[Row] = answer.map(prepareRow)
+ if (!isSorted) converted.sortBy(_.toString()) else converted
+ }
+
+ // We need to call prepareRow recursively to handle schemas with struct types.
+ private def prepareRow(row: Row): Row = {
+ Row.fromSeq(row.toSeq.map {
+ case null => null
+ case bd: java.math.BigDecimal => BigDecimal(bd)
+ // Equality of WrappedArray differs for AnyVal and AnyRef in Scala 2.12.2+
+ case seq: Seq[_] => seq.map {
+ case b: java.lang.Byte => b.byteValue
+ case s: java.lang.Short => s.shortValue
+ case i: java.lang.Integer => i.intValue
+ case l: java.lang.Long => l.longValue
+ case f: java.lang.Float => f.floatValue
+ case d: java.lang.Double => d.doubleValue
+ case x => x
+ }
+ // Convert array to Seq for easy equality check.
+ case b: Array[_] => b.toSeq
+ case r: Row => prepareRow(r)
+ // SPARK-51349: "null" and null had the same precedence in sorting
+ case "null" => "__null_string__"
+ case o => o
+ })
+ }
+
+ private def genError(
+ expectedAnswer: Seq[Row],
+ sparkAnswer: Seq[Row],
+ isSorted: Boolean = false): String = {
+ val getRowType: Option[Row] => String = row =>
+ row.map(row =>
+ if (row.schema == null) {
+ "struct<>"
+ } else {
+ s"${row.schema.catalogString}"
+ }).getOrElse("struct<>")
Review Comment:
This code is copy-pasted from `QueryTest`, I'd like to keep it as-is for now.
##########
sql/core/src/test/scala/org/apache/spark/sql/CheckAnswerHelper.scala:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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
+
+import java.util.TimeZone
+
+import org.scalatest.Assertions
+
+import org.apache.spark.annotation.Experimental
+import org.apache.spark.sql.catalyst.plans.logical
+import org.apache.spark.util.{SparkErrorUtils, SparkStringUtils}
+
+/**
+ * Provides [[checkAnswer]] helper for SQL- & DataFrame-API tests.
+ *
+ * TODO: should be moved to sql/api together with SessionQueryTestBase
+ */
+@Experimental
+trait CheckAnswerHelper extends Assertions {
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ protected def checkAnswer(df: => DataFrame, expectedAnswer: Seq[Row]): Unit = {
+ getErrorMessageInCheckAnswer(df, expectedAnswer) match {
+ case Some(errorMessage) => fail(errorMessage)
+ case None =>
+ }
+ }
+
+ /*
+ * Note: when moving this to sql/api, implementation should stay in sql/core
+ * (i.e. only have abstract decl in sql/api)
+ */
+ protected def isDfSorted(df: DataFrame): Boolean = {
+ df match {
+ case df: classic.DataFrame =>
+ df.logicalPlan.collectFirst { case s: logical.Sort => s }.nonEmpty
+ case _ => throw new RuntimeException(s"Cannot determine whether df is sorted: $df")
+ }
+ }
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ * If there was exception during the execution or the contents of the DataFrame does not
Review Comment:
This code is copy-pasted from `QueryTest`, I'd like to keep it as-is for now.
##########
core/src/test/scala/org/apache/spark/CheckErrorHelper.scala:
##########
@@ -0,0 +1,206 @@
+/*
+ * 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
+
+import scala.collection.mutable.ListBuffer
+import scala.jdk.CollectionConverters._
+
+import org.scalatest.Suite
+
+trait CheckErrorHelper { self: Suite =>
+
+ case class ExpectedContext(
+ contextType: QueryContextType,
+ objectType: String,
+ objectName: String,
+ startIndex: Int,
+ stopIndex: Int,
+ fragment: String,
+ callSitePattern: String
+ )
+
+ object ExpectedContext {
+ def apply(fragment: String, start: Int, stop: Int): ExpectedContext = {
+ ExpectedContext("", "", start, stop, fragment)
+ }
+
+ // Check the fragment only. This is only used when the fragment is distinguished within
+ // the query text
+ def apply(fragment: String): ExpectedContext = {
+ ExpectedContext("", "", -1, -1, fragment)
+ }
+
+ def apply(
+ objectType: String,
+ objectName: String,
+ startIndex: Int,
+ stopIndex: Int,
+ fragment: String): ExpectedContext = {
+ new ExpectedContext(QueryContextType.SQL, objectType, objectName, startIndex, stopIndex,
+ fragment, "")
+ }
+
+ def apply(fragment: String, callSitePattern: String): ExpectedContext = {
+ new ExpectedContext(QueryContextType.DataFrame, "", "", -1, -1, fragment, callSitePattern)
+ }
+ }
+
+ /**
+ * Parameter keys that are omitted from comparison when absent from the expected map.
+ * For each error condition, the set lists keys that are removed from the actual
+ * exception parameters before comparison with the expected map.
+ * Test suites may override this to add or change ignorable parameters per condition.
+ */
+ protected def checkErrorIgnorableParameters: Map[String, Set[String]] = Map(
+ "TABLE_OR_VIEW_NOT_FOUND" -> Set("searchPath")
+ )
+
+ /**
+ * Checks an exception with an error condition against expected results.
+ * @param exception The exception to check
+ * @param condition The expected error condition identifying the error
+ * @param sqlState Optional the expected SQLSTATE, not verified if not supplied
+ * @param parameters A map of parameter names and values. The names are as defined
+ * in the error-classes file.
+ * @param matchPVals Optionally treat the parameters value as regular expression pattern.
+ * false if not supplied.
+ */
+ protected def checkError(
+ exception: SparkThrowable,
+ condition: String,
+ sqlState: Option[String] = None,
+ parameters: Map[String, String] = Map.empty,
+ matchPVals: Boolean = false,
+ queryContext: Array[ExpectedContext] = Array.empty): Unit = {
Review Comment:
This code is copy-pasted from `SparkTestSuite`, I'd like to keep it as-is for now.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3454477314
##########
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:
@cloud-fan @johanl-db @ionagamed I'm a bit unsure how we should handle config stuff:
currently, `withSQLConf` is used pervasively but `SQLConf` is not part of the public API, only `RuntimeConfig`.
I see the following options:
- keep the `withSQLConf` name but implement the `connect` override by manipulating `spark.conf` ('only' has type `RuntimeConfig`) (would be confusing)
- introduce `withRuntimeConf` as the 'new' way to set confs while deprecating `withSQLConf` at for 'advertisement'
Are there other options? What do you think?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3454200345
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/SparkSessionBinder.scala:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.connect
+
+import java.util.UUID
+
+import org.apache.spark.{SparkEnv, SparkFunSuite}
+import org.apache.spark.sql
+import org.apache.spark.sql.classic
+import org.apache.spark.sql.connect.client.SparkConnectClient
+import org.apache.spark.sql.connect.config.Connect
+import org.apache.spark.sql.connect.service.SparkConnectService
+
+/**
+ * Provides a [[SparkSession connect.SparkSession]] backed by an in-process gRPC server.
+ * Extends [[sql.SparkSessionBinder sql.SparkSessionBinder]] (which creates a
+ * [[classic.SparkSession classic.SparkSession]] and SparkContext), then layers a Connect client
+ * session on top by starting the gRPC service in-process.
+ */
+trait SparkSessionBinder extends sql.SparkSessionBinder { self: SparkFunSuite =>
+
+ private var _connectSpark: SparkSession = _
+
+ protected override def spark: SparkSession = _connectSpark
+
+ /** The underlying classic session used by the in-process server. */
+ protected def classicSpark: classic.SparkSession = super.spark.asInstanceOf[classic.SparkSession]
+
+ override protected def beforeAll(): Unit = {
+ super.beforeAll()
+ // Other suites using mocks leave a mess in the global executionManager,
+ // shut it down so that it's cleared before starting server.
+ SparkConnectService.executionManager.shutdown()
+ val prevPort = SparkEnv.get.conf.get(Connect.CONNECT_GRPC_BINDING_PORT)
+ try {
+ // set GRPC_BINDING_PORT to 0 so that the server picks a random, freely available port.
+ SparkEnv.get.conf.set(Connect.CONNECT_GRPC_BINDING_PORT, 0)
+ SparkConnectService.start(classicSpark.sparkContext)
+ } finally {
+ SparkEnv.get.conf.set(Connect.CONNECT_GRPC_BINDING_PORT, prevPort)
+ }
+ }
+
+ override def beforeEach(): Unit = {
+ val client = SparkConnectClient
Review Comment:
Discussed here: https://github.com/apache/spark/pull/56190#discussion_r3392033491, though I'm unsure what'd be the right default?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3453769069
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/ExampleConnectSuite.scala:
##########
@@ -0,0 +1,22 @@
+/*
+ * 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.connect
+
+import org.apache.spark.sql
+
+class ExampleConnectSuite extends sql.ExampleSuite with SessionQueryTest
Review Comment:
> A single page example showing everything will also help sharing knowledge of how to use this
Wenchen suggested editing the `AGENTS.md` to document this. Furthermore, we could link there from the [Contributing to Spark](https://spark.apache.org/contributing.html) guide or even write a 'How to write Tests for Spark SQL / DataFrame stuff' thingy.
I also intend to post to the dev mailing list once this PR has landed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3453724378
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTest.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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
+
+import org.apache.spark.SparkFunSuite
+
+/**
+ * Provides connect-compatible test utils to write suites that have 'connect variants':
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
+ *
+ * // in sql/connect
+ * FooConnectSuite extends connect.SessionQueryTest
+ * }}}
+ *
+ * While this trait internally uses a [[classic.SparkSession]] when executing tests,
+ * it exposed as a [[SparkSession sql.SparkSession]] to allow for overriding on the connect side.
+ *
+ * For classic-specific tests, use [[classic.SessionQueryTest]].
+ */
+trait SessionQueryTest
+ extends SparkFunSuite
+ with SessionQueryTestBase
+ with SparkSessionBinder {
+
+ override def sessionType: String = "classic"
Review Comment:
With this, tests can handle and document session-specific behaviour:
```
test(...) {
val df = // query with connect-specific behaviour
if (sessionType == "connect") {
checkError(...)
} else {
checkAnswer(df, ...)
}
}
```
This is explained in the docstring in `SessionQueryTestBase`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3454500734
##########
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:
@cloud-fan @ionagamed @johanl-db I'm unsure wether we should immediately deprecate `SharedSparkSession` or only do that in e.g. ~1 month after we spent some time using `SessionQueryTest` ourselves.
Maybe it's better if we find potential 'UX gaps' ourselves 😅
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3454200345
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/SparkSessionBinder.scala:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.connect
+
+import java.util.UUID
+
+import org.apache.spark.{SparkEnv, SparkFunSuite}
+import org.apache.spark.sql
+import org.apache.spark.sql.classic
+import org.apache.spark.sql.connect.client.SparkConnectClient
+import org.apache.spark.sql.connect.config.Connect
+import org.apache.spark.sql.connect.service.SparkConnectService
+
+/**
+ * Provides a [[SparkSession connect.SparkSession]] backed by an in-process gRPC server.
+ * Extends [[sql.SparkSessionBinder sql.SparkSessionBinder]] (which creates a
+ * [[classic.SparkSession classic.SparkSession]] and SparkContext), then layers a Connect client
+ * session on top by starting the gRPC service in-process.
+ */
+trait SparkSessionBinder extends sql.SparkSessionBinder { self: SparkFunSuite =>
+
+ private var _connectSpark: SparkSession = _
+
+ protected override def spark: SparkSession = _connectSpark
+
+ /** The underlying classic session used by the in-process server. */
+ protected def classicSpark: classic.SparkSession = super.spark.asInstanceOf[classic.SparkSession]
+
+ override protected def beforeAll(): Unit = {
+ super.beforeAll()
+ // Other suites using mocks leave a mess in the global executionManager,
+ // shut it down so that it's cleared before starting server.
+ SparkConnectService.executionManager.shutdown()
+ val prevPort = SparkEnv.get.conf.get(Connect.CONNECT_GRPC_BINDING_PORT)
+ try {
+ // set GRPC_BINDING_PORT to 0 so that the server picks a random, freely available port.
+ SparkEnv.get.conf.set(Connect.CONNECT_GRPC_BINDING_PORT, 0)
+ SparkConnectService.start(classicSpark.sparkContext)
+ } finally {
+ SparkEnv.get.conf.set(Connect.CONNECT_GRPC_BINDING_PORT, prevPort)
+ }
+ }
+
+ override def beforeEach(): Unit = {
+ val client = SparkConnectClient
Review Comment:
Discussed here: https://github.com/apache/spark/pull/56190#discussion_r3392033491, though I'm unsure what'd be the right default 🤔
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "ionagamed (via GitHub)" <gi...@apache.org>.
ionagamed commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3455538435
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTest.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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
+
+import org.apache.spark.SparkFunSuite
+
+/**
+ * Provides connect-compatible test utils to write suites that have 'connect variants':
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
+ *
+ * // in sql/connect
+ * FooConnectSuite extends connect.SessionQueryTest
+ * }}}
+ *
+ * While this trait internally uses a [[classic.SparkSession]] when executing tests,
+ * it exposed as a [[SparkSession sql.SparkSession]] to allow for overriding on the connect side.
+ *
+ * For classic-specific tests, use [[classic.SessionQueryTest]].
+ */
+trait SessionQueryTest
+ extends SparkFunSuite
+ with SessionQueryTestBase
+ with SparkSessionBinder {
+
+ override def sessionType: String = "classic"
Review Comment:
Agreeing with:
> If callers really want to know what type of session they have, they can use reflection
This seems to go against the spirit of writing session-agnostic tests.
The suggested test might be implemented cleaner as:
```scala
class FooClassicSuite
extends classic.SessionQueryTest {
test(...) {
val df = // query with connect-specific-behavior
checkError(...)
}
}
class FooConnectSuite
extends connect.SessionQueryTest {
test(...) {
val df = // query with connect-specific-behavior
checkAnswer(df, ...)
}
}
```
or even:
```scala
class FooBaseSuite {
def bar() = // some meaningful actions isolated as a function
}
class FooClassicSuite
extends FooBaseSuite
with classic.SessionQueryTest {
test(...) {
bar()
checkError(...)
}
}
class FooConnectSuite
extends FooBaseSuite
with connect.SessionQueryTest {
test(...) {
bar()
checkAnswer(df, ...)
}
}
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SparkSessionBinder to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3326070806
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -1211,7 +1211,7 @@ object QueryTest extends Assertions {
}
-class QueryTestSuite extends test.SharedSparkSession {
+class QueryTestSuite extends QueryTest with SparkSessionBinder {
Review Comment:
I want to nudge test authors towards writing (somewhat) connect-compatible tests by default, which is why I want them to write tests with a `sql.SparkSession` in hand.
My fear is that the 'clean' way is not the 'easiest' way. Most current tests do not use an abstract base class and I fear that most test authors will default to just start a new suite with `classic.SparkSessionBinder` as they might not think about `connect` in that moment:
```scala
// hypothetical antipattern, but path of least resistance:
class FooSuite extends QueryTest with classic.SparkSessionBinder {
test("all tests, both shared and classic only") { ... }
}
```
I reworked the PR so that `SparkSessionBinder` now implements `QueryTest`. Now `classic.SparkSessionBinder` is a drop-in replacement for `SharedSparkSession` and `sql.SparkSessionBinder` provides the new, 'fixed' default.
What do you think of this approach?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3493073344
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTestBase.scala:
##########
@@ -0,0 +1,80 @@
+/*
+ * 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
+// 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 QueryCleanupHelper {
+
+ /**
+ * Sets all configurations specified in `pairs`, calls `f`, and then restores all configurations.
+ *
+ * Use this instead of `withSQLConf` as [[internal.SQLConf SQLConf]] is not part of Spark's public
+ * API.
+ */
+ protected def withConf[T](pairs: (String, String)*)(f: => T): T = {
+ val (keys, values) = pairs.unzip
+ val currentValues = keys.map { key =>
+ if (spark.conf.contains(key)) {
+ Some(spark.conf.get(key))
+ } else {
+ None
+ }
+ }
+ keys.lazyZip(values).foreach { (k, v) =>
+ spark.conf.set(k, v)
+ }
+ try f finally {
+ keys.zip(currentValues).foreach {
+ case (key, Some(value)) => spark.conf.set(key, value)
+ case (key, None) => spark.conf.unset(key)
+ }
+ }
+ }
+
+ /**
+ * Documents used session so that tests can handle and document session-specific behaviour
+ *
+ * {{{
+ * test(...) {
+ * val df = // query with connect-specific behaviour
+ * if (sessionType == "connect") {
+ * checkError(...)
+ * } else {
+ * checkAnswer(df, ...)
+ * }
+ * }
+ * }}}
+ */
+ def sessionType: String
Review Comment:
`sessionType: String` is typo-prone in exactly the way that bites silently: the idiom this trait blesses is `if (sessionType == "connect")`, and a misspelling (`"conenct"`, or even `"Connect"`) compiles and reads `false`, so a Connect run quietly takes the *classic* branch with no error. Since this is the standard hundreds of suites will copy, I'd make it a boolean:
```scala
def isConnect: Boolean
```
with `override def isConnect = true` only in `connect.SessionQueryTest` (the base/`classic` default `false`). `isConnect` polarity lets the divergent branch read positively — `if (isConnect) checkError(...) else checkAnswer(...)` — which is the case tests actually special-case. Call sites here move too: the `"classic"`/`"connect"` overrides, the `sessionType == "connect"` checks in `ExampleSessionAgnosticSuite`, and the scaladoc example above.
This is the simpler form of the sealed-`SessionType` idea from the 2026-06-23 thread — a boolean is enough for a strictly binary classic/connect split (an orthogonal axis like codegen-on/off would be its own flag anyway). Non-blocking.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "ionagamed (via GitHub)" <gi...@apache.org>.
ionagamed commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3491875866
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/SessionQueryTest.scala:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.connect
+
+import scala.util.matching.Regex
+
+import org.apache.spark.sql
+
+/**
+ * Overrides test utils to implement 'connect variants' of suites declared in sql/core:
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
+ *
+ * // in sql/connect
+ * FooConnectSuite extends FooSuite with connect.SessionQueryTest
+ * }}}
+ *
+ * This trait overrides [[spark]] to use a [[SparkSession connect.SparkSession]], which executes
+ * via the gRPC API using an in-process connect server.
+ */
+trait SessionQueryTest extends sql.SessionQueryTest with SparkSessionBinder {
+
+ private val sortOperator: Regex = "\b(?:Photon)?Sort\b".r
Review Comment:
Good catch! Thanks, fixed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3459895804
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -823,21 +789,39 @@ trait QueryTest extends SparkFunSuite with QueryTestBase {
}
}
-object QueryTest extends Assertions {
+@deprecated("superseded by CheckAnswerHelper", since = "4.2")
+object QueryTest extends CheckAnswerHelper {
Review Comment:
@cloud-fan IMHO we should either keep the duplication or use reflection to delegate to the private methods in `CheckAnswerHelper` because I think it would be very nice to keep the helpers in `CheckAnswer` private.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "ionagamed (via GitHub)" <gi...@apache.org>.
ionagamed commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3497239423
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTestBase.scala:
##########
@@ -0,0 +1,80 @@
+/*
+ * 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
+// 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 QueryCleanupHelper {
+
+ /**
+ * Sets all configurations specified in `pairs`, calls `f`, and then restores all configurations.
+ *
+ * Use this instead of `withSQLConf` as [[internal.SQLConf SQLConf]] is not part of Spark's public
+ * API.
+ */
+ protected def withConf[T](pairs: (String, String)*)(f: => T): T = {
+ val (keys, values) = pairs.unzip
+ val currentValues = keys.map { key =>
+ if (spark.conf.contains(key)) {
+ Some(spark.conf.get(key))
+ } else {
+ None
+ }
+ }
+ keys.lazyZip(values).foreach { (k, v) =>
+ spark.conf.set(k, v)
+ }
+ try f finally {
+ keys.zip(currentValues).foreach {
+ case (key, Some(value)) => spark.conf.set(key, value)
+ case (key, None) => spark.conf.unset(key)
+ }
+ }
+ }
+
+ /**
+ * Documents used session so that tests can handle and document session-specific behaviour
+ *
+ * {{{
+ * test(...) {
+ * val df = // query with connect-specific behaviour
+ * if (sessionType == "connect") {
+ * checkError(...)
+ * } else {
+ * checkAnswer(df, ...)
+ * }
+ * }
+ * }}}
+ */
+ def sessionType: String
Review Comment:
Agreed, thanks! Going with `isConnect` for now.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3454408680
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/DataSourceV2DataFrameConnectSuite.scala:
##########
@@ -34,7 +34,7 @@ import org.apache.spark.sql.connector.catalog.{CachingInMemoryTableCatalog, InMe
* this class only provides the Connect-specific session, catalog access, and result comparison.
*/
class DataSourceV2DataFrameConnectSuite
- extends SparkConnectServerTest
+ extends SessionQueryTest
Review Comment:
Is there a clear 'ideal way' for how which session semantics to provide? Per-testcase would imply quite a bit of overhead, right?
Maybe we could leave guarantees around sessions explicitly 'undefined' to for potential future optimizations?
(e.g. parallel execution of testscases in different session)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SparkSessionBinder to replace SharedSparkSession [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3329780696
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -1211,7 +1211,7 @@ object QueryTest extends Assertions {
}
-class QueryTestSuite extends test.SharedSparkSession {
+class QueryTestSuite extends QueryTest with SparkSessionBinder {
Review Comment:
Given we are setting up the standard test style, can you spell out your proposal clearly? If you don't agree with my abstract test suite proposal, what's your proposal? Can you write down example test suites so that people can understand you easier?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3454214218
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTest.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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
+
+import org.apache.spark.SparkFunSuite
+
+/**
+ * Provides connect-compatible test utils to write suites that have 'connect variants':
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
Review Comment:
Err yeah the docstring is wrong, should be `FooConnectSuite extends FooSuite with connect.SessionQueryTest` 😅. Fixed.
So `protected` utils can just be re-used.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3454286577
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/SessionQueryTest.scala:
##########
@@ -0,0 +1,24 @@
+/*
+ * 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.hive
+
+import org.apache.spark.sql.classic
+
+trait SessionQueryTest extends classic.SessionQueryTest with test.TestHiveSingleton {
Review Comment:
> drop the hive variant from this PR
Did this, adding a hive variant should be a follow-up
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3454477314
##########
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:
@cloud-fan @johanl-db I'm a bit unsure how we should handle config stuff:
currently, `withSQLConf` is used pervasively but `SQLConf` is not part of the public API, only `RuntimeConfig`.
I see the following options:
- keep the `withSQLConf` name but implement the `connect` override by manipulating `spark.conf` ('only' has type `RuntimeConfig`) (would be confusing)
- introduce `withRuntimeConf` as the 'new' way to set confs while deprecating `withSQLConf` at for 'advertisement'
Are there other options? What do you think?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "ionagamed (via GitHub)" <gi...@apache.org>.
ionagamed commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3455549918
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/SparkSessionBinder.scala:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.connect
+
+import java.util.UUID
+
+import org.apache.spark.{SparkEnv, SparkFunSuite}
+import org.apache.spark.sql
+import org.apache.spark.sql.classic
+import org.apache.spark.sql.connect.client.SparkConnectClient
+import org.apache.spark.sql.connect.config.Connect
+import org.apache.spark.sql.connect.service.SparkConnectService
+
+/**
+ * Provides a [[SparkSession connect.SparkSession]] backed by an in-process gRPC server.
+ * Extends [[sql.SparkSessionBinder sql.SparkSessionBinder]] (which creates a
+ * [[classic.SparkSession classic.SparkSession]] and SparkContext), then layers a Connect client
+ * session on top by starting the gRPC service in-process.
+ */
+trait SparkSessionBinder extends sql.SparkSessionBinder { self: SparkFunSuite =>
+
+ private var _connectSpark: SparkSession = _
+
+ protected override def spark: SparkSession = _connectSpark
+
+ /** The underlying classic session used by the in-process server. */
+ protected def classicSpark: classic.SparkSession = super.spark.asInstanceOf[classic.SparkSession]
+
+ override protected def beforeAll(): Unit = {
+ super.beforeAll()
+ // Other suites using mocks leave a mess in the global executionManager,
+ // shut it down so that it's cleared before starting server.
+ SparkConnectService.executionManager.shutdown()
+ val prevPort = SparkEnv.get.conf.get(Connect.CONNECT_GRPC_BINDING_PORT)
+ try {
+ // set GRPC_BINDING_PORT to 0 so that the server picks a random, freely available port.
+ SparkEnv.get.conf.set(Connect.CONNECT_GRPC_BINDING_PORT, 0)
+ SparkConnectService.start(classicSpark.sparkContext)
+ } finally {
+ SparkEnv.get.conf.set(Connect.CONNECT_GRPC_BINDING_PORT, prevPort)
+ }
+ }
+
+ override def beforeEach(): Unit = {
+ val client = SparkConnectClient
Review Comment:
I see. Might be worth a comment to reduce questions when reading through the code.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3459950360
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTest.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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
+
+import org.apache.spark.SparkFunSuite
+
+/**
+ * Provides connect-compatible test utils to write suites that have 'connect variants':
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
+ *
+ * // in sql/connect
+ * FooConnectSuite extends connect.SessionQueryTest
+ * }}}
+ *
+ * While this trait internally uses a [[classic.SparkSession]] when executing tests,
+ * it exposed as a [[SparkSession sql.SparkSession]] to allow for overriding on the connect side.
+ *
+ * For classic-specific tests, use [[classic.SessionQueryTest]].
+ */
+trait SessionQueryTest
+ extends SparkFunSuite
+ with SessionQueryTestBase
+ with SparkSessionBinder {
+
+ override def sessionType: String = "classic"
Review Comment:
UPDATE:
> If callers really want to know what type of session they have, they can use reflection
while discussing with @ionagamed, we realized that reflection doesn't really work now and would break even further when moving test definitons into `sql/tests`:
Inside `sql/core`, we cannot see `connect.SparkSession`, so we can only as 'is this a classic session', which is IMHO kinda limiting.
And when we move test definitions to some hypothetical `sql/tests` module that only sees `sql/api`, we can't even do that anymore.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3459950360
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTest.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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
+
+import org.apache.spark.SparkFunSuite
+
+/**
+ * Provides connect-compatible test utils to write suites that have 'connect variants':
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
+ *
+ * // in sql/connect
+ * FooConnectSuite extends connect.SessionQueryTest
+ * }}}
+ *
+ * While this trait internally uses a [[classic.SparkSession]] when executing tests,
+ * it exposed as a [[SparkSession sql.SparkSession]] to allow for overriding on the connect side.
+ *
+ * For classic-specific tests, use [[classic.SessionQueryTest]].
+ */
+trait SessionQueryTest
+ extends SparkFunSuite
+ with SessionQueryTestBase
+ with SparkSessionBinder {
+
+ override def sessionType: String = "classic"
Review Comment:
UPDATE:
> If callers really want to know what type of session they have, they can use reflection
while discussing with @ionagamed, we realized that ~reflection~ checking type info at runtime via e.g. `spark.isInstanceOf[connect.SparkSession]` doesn't really work now and would break even further when moving test definitons into `sql/tests`:
Inside `sql/core`, we cannot see `connect.SparkSession`, so we can only as 'is this a classic session', which is IMHO kinda limiting.
And when we move test definitions to some hypothetical `sql/tests` module that only sees `sql/api`, we can't even do that anymore.
EDIT: I mixed up reflection and runtime type inspection, corrected accordingly.
How would a `isConnect?` with reflection look like?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SparkSessionBinder to replace SharedSparkSession [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #56190:
URL: https://github.com/apache/spark/pull/56190#issuecomment-4618546717
https://github.com/apache/spark/pull/56190#discussion_r3329780696 can you address this comment? I'd like to see the final test suite style we want to support.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "ionagamed (via GitHub)" <gi...@apache.org>.
ionagamed commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3491350255
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTest.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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
+
+import org.apache.spark.SparkFunSuite
+
+/**
+ * Provides connect-compatible test utils to write suites that have 'connect variants':
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
+ *
+ * // in sql/connect
+ * FooConnectSuite extends connect.SessionQueryTest
+ * }}}
+ *
+ * While this trait internally uses a [[classic.SparkSession]] when executing tests,
+ * it exposed as a [[SparkSession sql.SparkSession]] to allow for overriding on the connect side.
+ *
+ * For classic-specific tests, use [[classic.SessionQueryTest]].
+ */
+trait SessionQueryTest
+ extends SparkFunSuite
+ with SessionQueryTestBase
+ with SparkSessionBinder {
+
+ override def sessionType: String = "classic"
Review Comment:
IMO this is not a one-way door. Given the precedent, I'd say that we should just go ahead with the `sessionType` that is already here.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3453774848
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTestBase.scala:
##########
@@ -0,0 +1,51 @@
+/*
+ * 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
+// 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 QueryCleanupHelper {
+
+ /**
+ * Documents used session so that tests can handle and document session-specific behaviour
+ *
+ * {{{
+ * test(...) {
+ * val df = // query with connect-specific behaviour
+ * if (sessionType == "connect") {
+ * checkError(...)
+ * } else {
+ * checkAnswer(df, ...)
+ * }
+ * }
+ * }}}
+ */
+ def sessionType: String
Review Comment:
answered here: https://github.com/apache/spark/pull/56190#discussion_r3453724378
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3457815865
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTest.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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
+
+import org.apache.spark.SparkFunSuite
+
+/**
+ * Provides connect-compatible test utils to write suites that have 'connect variants':
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
+ *
+ * // in sql/connect
+ * FooConnectSuite extends connect.SessionQueryTest
+ * }}}
+ *
+ * While this trait internally uses a [[classic.SparkSession]] when executing tests,
+ * it exposed as a [[SparkSession sql.SparkSession]] to allow for overriding on the connect side.
+ *
+ * For classic-specific tests, use [[classic.SessionQueryTest]].
+ */
+trait SessionQueryTest
+ extends SparkFunSuite
+ with SessionQueryTestBase
+ with SparkSessionBinder {
+
+ override def sessionType: String = "classic"
Review Comment:
I added `sessionType` since this pattern is already used in the classic/connect-agnostic tests here: https://github.com/apache/spark/blob/d76d7f5f6367518b4cb96b796a2c6ac83ec8303a/sql/core/src/test/scala/org/apache/spark/sql/connector/DSv2IncrementallyConstructedQueryTests.scala#L201-L232
> This seems to go against the spirit of writing session-agnostic tests.
AkShUaLLy, diverging classic-/connect-semantics go against the spirit of writing session-agnostic tests.
Having a `sessionType` here is a coping mechanism, and IMHO a good one.
> The suggested test might be implemented cleaner as [classic- and connect-specific test, potentially with shared outlined logic]
I strongly disagree here:
When writing a test to document diverging classic/connect behaviour, that should be _a single testcase_. Your example would generally be in two different files, in two different modules (`sql/core` and `sql/connect/server`). Ofc it is generally possible to view two files side-by-side, but it still requires the linking of 'oh yeah, the second half of this test is somewhere over there'.
(And outlining shared logic into `bar` violates my understanding of [DAMP, not DRY](https://abseil.io/resources/swe-book/html/ch12.html#tests_and_code_sharing_dampcomma_not_dr).)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3462916540
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -1223,4 +1212,19 @@ class QueryTestSuite extends test.SharedSparkSession {
"from range(2)"),
Seq(Row(Row(null)), Row(Row("null"))))
}
+
+ test("checkAnswer demands correct result order for ordered queries") {
+ val e = intercept[org.scalatest.exceptions.TestFailedException] {
+ checkAnswer(
+ sql("SELECT col1 FROM VALUE 1, 2, 1, 3 ORDER BY col 1"),
Review Comment:
Yeah, awkward typos 🙃
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #56190:
URL: https://github.com/apache/spark/pull/56190#issuecomment-4843657758
I've updated pr desc, thanks, merging to master/4.x!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SparkSessionBinder to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on PR #56190:
URL: https://github.com/apache/spark/pull/56190#issuecomment-4578383847
Hi @cloud-fan, I changed the PR so that `SharedSparkSession` is now an empty alias of `classic.SparkSessionBinder` with a deprecation note that recommends using `sql.SparkSessionBinder` if possible.
I am unsure with regards to the `SparkSessionBinder` name:
AFAICS `SharedSparkSession` is/was _the_ testing trait (~500 extends/implements usages compared to ~150 usages of `QueryTest`, both according to Intellij's "Find Usages").
If it wouldn't be a breaking change, I'd want to rename `QueryTest` to `QueryTestHelpers` and `SparkSessionBinder` to `QueryTest`. What do you think? Maybe `QuerySuite`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3454500734
##########
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:
@cloud-fan @ionagamed @johanl-db I'm unsure wether we should immediately deprecate `SharedSparkSession` or only do that after we spent some time using `SessionQueryTest` ourselves.
Maybe it's better if we find potential 'UX gaps' ourselves 😅
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3456225239
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -172,9 +173,15 @@ trait QueryTestBase
}
}
- assertEmptyMissingInput(analyzedDF)
+ if (analyzedDF.isInstanceOf[classic.DataFrame]) {
+ assertEmptyMissingInput(analyzedDF)
- QueryTest.checkAnswer(analyzedDF, expectedAnswer)
+ SQLExecution.withSQLConfPropagated(analyzedDF.sparkSession) {
+ df.materializedRdd.count() // Also attempt to deserialize as an RDD [SPARK-15791]
Review Comment:
`df` is the by-name parameter (`df: => DataFrame`), already forced once into `analyzedDF` at the top of this method. `df.materializedRdd` re-evaluates the thunk, so the query plan is constructed twice for every classic `checkAnswer` (and a side-effecting by-name block would run twice). Before this PR the trait evaluated `df` exactly once. Use the already-forced value:
```suggestion
analyzedDF.materializedRdd.count() // Also attempt to deserialize as an RDD [SPARK-15791]
```
##########
sql/core/src/test/scala/org/apache/spark/sql/CheckAnswerHelper.scala:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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
+
+import java.util.TimeZone
+
+import org.scalatest.Assertions
+
+import org.apache.spark.annotation.Experimental
+import org.apache.spark.sql.catalyst.ExtendedAnalysisException
+import org.apache.spark.sql.catalyst.plans.logical
+import org.apache.spark.util.{SparkErrorUtils, SparkStringUtils}
+
+/**
+ * Provides [[checkAnswer]] helper for SQL- & DataFrame-API tests.
+ *
+ * TODO: should be moved to sql/api together with SessionQueryTestBase
+ */
+@Experimental
+trait CheckAnswerHelper extends Assertions {
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ protected def checkAnswer(df: => DataFrame, expectedAnswer: Seq[Row]): Unit = {
+
+ val analyzedDF = try df catch {
+ case ae: ExtendedAnalysisException =>
+ if (ae.plan.isDefined) {
+ fail(
+ s"""
+ |Failed to analyze query: $ae
+ |${ae.plan.get}
+ |
+ |${SparkErrorUtils.stackTraceToString(ae)}
+ |""".stripMargin)
+ } else {
+ throw ae
+ }
+ }
+
+ getErrorMessageInCheckAnswer(analyzedDF, expectedAnswer) match {
+ case Some(errorMessage) => fail(errorMessage)
+ case None =>
+ }
+ }
+
+ /*
+ * Note: when moving this to sql/api, implementation should stay in sql/core
+ * (i.e. only have abstract decl in sql/api)
+ */
+ protected def isDfSorted(df: DataFrame): Boolean = {
+ df match {
+ case df: classic.DataFrame =>
+ df.logicalPlan.collectFirst { case s: logical.Sort => s }.nonEmpty
+ case _ => throw new RuntimeException(s"Cannot determine whether df is sorted: $df")
Review Comment:
`checkAnswer` here flows to `getErrorMessageInCheckAnswer` → `sameRows(..., isDfSorted(df))`, and this default throws for any non-`classic.DataFrame`. The connect-safe override (`= false`) lives only in `connect.SessionQueryTest`, so the documented `FooSuite with connect.SparkSessionBinder` swap — what `QueryTestWithConnectSuite` does — hits this throw on every Connect `checkAnswer`, and that suite is red in CI for exactly this reason. Same trait-linearization issue raised on `QueryTest.scala:160`, in a method that fix didn't reach.
Returning `false` here (matching what `connect.SessionQueryTest` already does deliberately) makes the helper connect-safe by default, fixes the binder-swap path, and makes the `connect.SessionQueryTest.isDfSorted` override redundant — i.e. it consolidates the connect behavior into one mixin.
```suggestion
case _ => false
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3462910482
##########
sql/core/src/test/scala/org/apache/spark/sql/CheckAnswerHelper.scala:
##########
@@ -0,0 +1,211 @@
+/*
+ * 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
+
+import java.util.TimeZone
+
+import org.scalatest.Assertions
+
+import org.apache.spark.annotation.Experimental
+import org.apache.spark.sql.catalyst.ExtendedAnalysisException
+import org.apache.spark.sql.catalyst.plans.logical
+import org.apache.spark.util.{SparkErrorUtils, SparkStringUtils}
+
+/**
+ * Provides [[checkAnswer]] helper for SQL- & DataFrame-API tests.
+ *
+ * TODO: should be moved to sql/api together with SessionQueryTestBase
+ */
+@Experimental
+trait CheckAnswerHelper extends Assertions {
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ protected def checkAnswer(df: => DataFrame, expectedAnswer: Seq[Row]): Unit = {
+
+ val analyzedDF = try df catch {
+ case ae: ExtendedAnalysisException =>
+ if (ae.plan.isDefined) {
+ fail(
+ s"""
+ |Failed to analyze query: $ae
+ |${ae.plan.get}
+ |
+ |${SparkErrorUtils.stackTraceToString(ae)}
+ |""".stripMargin)
+ } else {
+ throw ae
+ }
+ }
+
+ getErrorMessageInCheckAnswer(analyzedDF, expectedAnswer) match {
+ case Some(errorMessage) => fail(errorMessage)
+ case None =>
+ }
+ }
+
+ /*
+ * Note: when moving this to sql/api, implementation should stay in sql/core
+ * (i.e. only have abstract decl in sql/api)
+ */
+ protected def isDfSorted(df: DataFrame): Boolean = {
+ df match {
+ case df: classic.DataFrame =>
+ df.logicalPlan.collectFirst { case s: logical.Sort => s }.nonEmpty
+ case _ => throw new RuntimeException(s"Cannot determine whether df is sorted: $df")
Review Comment:
I'd like to keep this 'connect unsafe' to prevent cases when users forget to mixin `connect.SessionQueryTest` (just like I forgot in the case of the error here).
I extended the error msg and added a comment.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "ionagamed (via GitHub)" <gi...@apache.org>.
ionagamed commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3491876946
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/SessionQueryTest.scala:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.connect
+
+import scala.util.matching.Regex
+
+import org.apache.spark.sql
+
+/**
+ * Overrides test utils to implement 'connect variants' of suites declared in sql/core:
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
+ *
+ * // in sql/connect
+ * FooConnectSuite extends FooSuite with connect.SessionQueryTest
+ * }}}
+ *
+ * This trait overrides [[spark]] to use a [[SparkSession connect.SparkSession]], which executes
+ * via the gRPC API using an in-process connect server.
+ */
+trait SessionQueryTest extends sql.SessionQueryTest with SparkSessionBinder {
+
+ private val sortOperator: Regex = "\b(?:Photon)?Sort\b".r
+
+ /**
+ * Approximates [[sql.SessionQueryTest.isDfSorted]] by inspecting the explain string.
+ */
+ override def isDfSorted(df: sql.DataFrame): Boolean = df match {
+ case df: DataFrame => sortOperator.matches(df.explainString(extended = false))
Review Comment:
Thanks! Fixed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "ionagamed (via GitHub)" <gi...@apache.org>.
ionagamed commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3491871737
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -202,6 +203,7 @@ trait QueryTestBase
* @param expectedAnswer the expected result in a [[Seq]] of [[Row]]s.
* @param absTol the absolute tolerance between actual and expected answers.
*/
+ @deprecated("rarely used", since = "4.2.0")
Review Comment:
Agreed, we're pushing the deprecation into a future PR.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SparkSessionBinder to replace SharedSparkSession [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3322066761
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -1211,7 +1211,7 @@ object QueryTest extends Assertions {
}
-class QueryTestSuite extends test.SharedSparkSession {
+class QueryTestSuite extends QueryTest with SparkSessionBinder {
Review Comment:
This migration — mixing the bare `sql.SparkSessionBinder` into a concrete suite — is the shape I'd push back on. `sql.SparkSessionBinder` binds a classic session but exposes `spark` only as the abstract `sql.SparkSession`, so it's really internal plumbing, not what a test author should reach for.
The end-state I'd recommend documenting and demonstrating is a binder-free base + per-env concrete suites:
```scala
abstract class FooSuiteBase extends QueryTest { // no binder; spark abstract
test("shared") { checkAnswer(sql("SELECT 1"), Row(1)) }
}
class FooSuite extends FooSuiteBase with classic.SparkSessionBinder {
test("classic only") { ... }
}
class FooConnectSuite extends FooSuiteBase
with connect.SparkSessionBinder with connect.QueryTest {
test("connect only") { ... }
}
```
`QueryTest` already mixes in `SparkSessionProvider` (via `SQLTestData`) and leaves `spark` abstract, so it works as the env-agnostic base directly. Concretely: (1) steer the migration and the `@deprecated` message at `classic.SparkSessionBinder` / `connect.SparkSessionBinder` + this base pattern, not the bare binder; (2) `QueryTestWithConnectSuite` currently demonstrates the retrofit path (extending an already-classic-bound `QueryTestSuite` and overriding the binding) — a binder-free base would demonstrate the cleaner pattern and double as the template authors copy.
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/QueryTest.scala:
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.connect
+
+import org.apache.spark.{sql => sqlApi}
+
+/**
+ * Extends [[sqlApi.QueryTest]] for use with Connect sessions.
+ *
+ * Overrides [[checkAnswer]] to avoid classic-only code paths (e.g. `queryExecution`,
+ * `logicalPlan`, `materializedRdd`) that are not available on Connect DataFrames.
+ */
+trait QueryTest extends sqlApi.QueryTest with SparkSessionProvider {
+
+ override protected def checkAnswer(
Review Comment:
This overrides only the `checkAnswer(df, Seq[Row])` variant, which is enough for `QueryTestSuite`. But the stated goal is re-running arbitrary `sql/core` suites over Connect, and the other `QueryTest` helpers (other `checkAnswer` overloads, `checkDataset`, ...) still reach classic-only paths like `queryExecution`/`logicalPlan`. Worth a line in the trait doc noting that broader reuse will need more overrides.
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/SparkSessionBinder.scala:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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.connect
+
+import java.util.UUID
+
+import scala.concurrent.duration._
+
+import org.scalatest.concurrent.Eventually
+
+import org.apache.spark.DebugFilesystem
+import org.apache.spark.sql
+import org.apache.spark.sql.classic
+import org.apache.spark.sql.connect.client.SparkConnectClient
+import org.apache.spark.sql.connect.common.config.ConnectCommon
+import org.apache.spark.sql.connect.config.Connect
+import org.apache.spark.sql.connect.service.SparkConnectService
+
+/**
+ * A test trait that provides a Connect [[SparkSession]] backed by an in-process gRPC server.
+ * Extends [[sql.SparkSessionBinder sql.SparkSessionBinder]] (which creates a
+ * [[classic.SparkSession classic.SparkSession]] and SparkContext), then layers a Connect client
+ * session on top by starting the gRPC service in-process.
+ *
+ * Mix in this trait to exercise existing sql/core test suites through the Connect path:
+ * {{{
+ * class FooWithConnectSuite
+ * extends FooSuite
+ * with connect.SparkSessionBinder
+ * with connect.QueryTest
+ * }}}
+ */
+trait SparkSessionBinder extends sql.SparkSessionBinder {
+
+ private val serverPort: Int =
+ ConnectCommon.CONNECT_GRPC_BINDING_PORT + util.Random.nextInt(1000)
+
+ @volatile private var _connectSpark: SparkSession = _
+
+ protected override def spark: SparkSession = _connectSpark
+
+ /** The underlying classic session used by the in-process server. */
+ private def classicSpark: classic.SparkSession = super.spark.asInstanceOf[classic.SparkSession]
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ withSparkEnvConfs((Connect.CONNECT_GRPC_BINDING_PORT.key, serverPort.toString)) {
+ SparkConnectService.start(classicSpark.sparkContext)
+ }
+ val client = SparkConnectClient
+ .builder()
+ .port(serverPort)
+ .sessionId(UUID.randomUUID().toString)
+ .userId("test")
+ .build()
+ _connectSpark = SparkSession
+ .builder()
+ .client(client)
+ .create()
+ }
+
+ override def afterAll(): Unit = {
+ try {
+ if (_connectSpark != null) {
+ _connectSpark.close()
+ _connectSpark = null
+ }
+ SparkConnectService.stop()
+ } finally {
+ super.afterAll()
+ }
+ }
+
+ // The base SharedSparkSessionBase.afterEach calls spark.sharedState which is not supported
Review Comment:
This comment is inaccurate after the refactor: `connect.SparkSessionBinder` extends `sql.SparkSessionBinder` directly (not `SharedSparkSessionBase`), and that parent's `afterEach` clears the cache via the private `_spark` — the classic session, which is exactly what's used on Connect (`createSparkSession` isn't overridden). So the parent's `afterEach` already works here and this override is redundant. If you do keep it, note that skipping `super.afterEach()` drops the `BeforeAndAfterEach` chain. Simplest fix is to remove the override entirely.
##########
sql/core/src/test/scala/org/apache/spark/sql/test/SharedSparkSession.scala:
##########
@@ -19,41 +19,12 @@ package org.apache.spark.sql.test
import scala.concurrent.duration._
-import org.scalatest.{BeforeAndAfterEach, Suite}
-import org.scalatest.concurrent.Eventually
-
-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
+import org.apache.spark.sql.{classic, QueryTest, QueryTestBase}
+@deprecated("Use SparkSessionBinder and QueryTest instead")
Review Comment:
`@deprecated` takes a `since` version as its second argument; adding it documents when the deprecation started and matches the convention elsewhere in the codebase. Same on line 59.
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/QueryTestWithConnectSuite.scala:
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.connect
+
+import org.apache.spark.sql.QueryTestSuite
+
+/**
+ * Runs [[QueryTestSuite]] tests through a Connect session.
+ *
+ * This validates the `FooSuite with connect.SharedSparkSession` pattern: the existing
Review Comment:
There's no `connect.SharedSparkSession` trait; the pattern this suite actually uses (and that the sibling `connect/SparkSessionBinder.scala` doc shows) is `connect.SparkSessionBinder with connect.QueryTest`.
```suggestion
* This validates the `FooSuite with connect.SparkSessionBinder with connect.QueryTest` pattern: the existing
```
##########
sql/core/src/test/scala/org/apache/spark/sql/SparkSessionBinder.scala:
##########
@@ -0,0 +1,163 @@
+/*
+ * 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
+
+import scala.concurrent.duration._
+
+import org.scalatest.{BeforeAndAfterEach, Suite}
+import org.scalatest.concurrent.Eventually
+
+import org.apache.spark.{DebugFilesystem, SparkConf, SparkFunSuite}
+import org.apache.spark.internal.config.UNSAFE_EXCEPTION_ON_MEMORY_LEAK
+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.test.TestSparkSession
+
+trait SparkSessionBinder
+ extends SparkFunSuite
+ with SparkSessionProvider
+ with BeforeAndAfterEach
+ with Eventually {
+
+ protected def sparkConf = {
+ val conf = new SparkConf()
+ .set("spark.hadoop.fs.file.impl", classOf[DebugFilesystem].getName)
+ .set(UNSAFE_EXCEPTION_ON_MEMORY_LEAK, true)
+ .set(SQLConf.CODEGEN_FALLBACK.key, "false")
+ .set(SQLConf.CODEGEN_FACTORY_MODE.key, CodegenObjectFactoryMode.CODEGEN_ONLY.toString)
+ // Disable ConvertToLocalRelation for better test coverage. Test cases built on
+ // LocalRelation will exercise the optimization rules better by disabling it as
+ // this rule may potentially block testing of other optimization rules such as
+ // ConstantPropagation etc.
+ .set(SQLConf.OPTIMIZER_EXCLUDED_RULES.key, ConvertToLocalRelation.ruleName)
+ conf.set(
+ StaticSQLConf.WAREHOUSE_PATH,
+ conf.get(StaticSQLConf.WAREHOUSE_PATH) + "/" + getClass.getCanonicalName)
+ conf.set(StaticSQLConf.LOAD_SESSION_EXTENSIONS_FROM_CLASSPATH, false)
+ conf.set(StaticSQLConf.SHUFFLE_EXCHANGE_MAX_THREAD_THRESHOLD,
+ sys.env.getOrElse("SPARK_TEST_SQL_SHUFFLE_EXCHANGE_MAX_THREAD_THRESHOLD",
+ StaticSQLConf.SHUFFLE_EXCHANGE_MAX_THREAD_THRESHOLD.defaultValueString).toInt)
+ conf.set(StaticSQLConf.RESULT_QUERY_STAGE_MAX_THREAD_THRESHOLD,
+ sys.env.getOrElse("SPARK_TEST_SQL_RESULT_QUERY_STAGE_MAX_THREAD_THRESHOLD",
+ StaticSQLConf.RESULT_QUERY_STAGE_MAX_THREAD_THRESHOLD.defaultValueString).toInt)
+ }
+
+ /**
+ * The [[TestSparkSession]] to use for all tests in this suite.
+ *
+ * By default, the underlying [[org.apache.spark.SparkContext]] will be run in local
+ * mode with the default test configurations.
+ */
+ private var _spark: classic.SparkSession = null
+
+ protected override def spark: SparkSession = _spark
+
+ /**
+ * The [[TestSQLContext]] to use for all tests in this suite.
+ */
+ protected implicit def sqlContext: SQLContext = _spark.sqlContext
+
+ protected def createSparkSession: classic.SparkSession = {
+ classic.SparkSession.cleanupAnyExistingSession()
+ new TestSparkSession(sparkConf)
+ }
+
+ protected def sqlConf: SQLConf = _spark.sessionState.conf
+
+ /**
+ * Initialize the [[TestSparkSession]]. Generally, this is just called from
+ * beforeAll; however, in test using styles other than FunSuite, there is
+ * often code that relies on the session between test group constructs and
+ * the actual tests, which may need this session. It is purely a semantic
+ * difference, but semantically, it makes more sense to call
+ * 'initializeSession' between a 'describe' and an 'it' call than it does to
+ * call 'beforeAll'.
+ */
+ protected def initializeSession(): Unit = {
+ if (_spark == null) {
+ _spark = createSparkSession
+ }
+ }
+
+ /**
+ * Suites extending [[SharedSparkSession]] are sharing resources (e.g. SparkSession) in their
Review Comment:
This doc moved out of `SharedSparkSession`; the snapshot-before-init logic now lives in this trait, so referring to `SharedSparkSession` is stale.
```suggestion
* Suites extending this trait are sharing resources (e.g. SparkSession) in their
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "ionagamed (via GitHub)" <gi...@apache.org>.
ionagamed commented on PR #56190:
URL: https://github.com/apache/spark/pull/56190#issuecomment-4841347152
I don't have write permissions for the PR description, waiting for @fwc to address the description suggestions.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "ionagamed (via GitHub)" <gi...@apache.org>.
ionagamed commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3491879866
##########
sql/connect/common/src/main/scala/org/apache/spark/sql/connect/Dataset.scala:
##########
@@ -241,6 +241,29 @@ class Dataset[T] private[sql] (
// scalastyle:on println
}
+ private[connect] def explainString(mode: String): String = {
+ val protoMode = mode.trim.toLowerCase(util.Locale.ROOT) match {
+ case "simple" => proto.AnalyzePlanRequest.Explain.ExplainMode.EXPLAIN_MODE_SIMPLE
+ case "extended" => proto.AnalyzePlanRequest.Explain.ExplainMode.EXPLAIN_MODE_EXTENDED
+ case "codegen" => proto.AnalyzePlanRequest.Explain.ExplainMode.EXPLAIN_MODE_CODEGEN
+ case "cost" => proto.AnalyzePlanRequest.Explain.ExplainMode.EXPLAIN_MODE_COST
+ case "formatted" => proto.AnalyzePlanRequest.Explain.ExplainMode.EXPLAIN_MODE_FORMATTED
+ case _ => throw new IllegalArgumentException("Unsupported explain mode: " + mode)
+ }
+ sparkSession
+ .analyze(plan, proto.AnalyzePlanRequest.AnalyzeCase.EXPLAIN, Some(protoMode))
+ .getExplain
+ .getExplainString
+ }
+
+ private[connect] def explainString(extended: Boolean): String = if (extended) {
+ explainString("extended")
+ } else {
+ explainString("simple")
+ }
+
+ private[connect] def explainString(): String = explainString("simple")
Review Comment:
Agreed, dropped.
##########
sql/core/src/test/scala/org/apache/spark/sql/CheckAnswerHelper.scala:
##########
@@ -0,0 +1,215 @@
+/*
+ * 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
+
+import java.util.TimeZone
+
+import org.scalatest.Assertions
+
+import org.apache.spark.annotation.Experimental
+import org.apache.spark.sql.catalyst.ExtendedAnalysisException
+import org.apache.spark.sql.catalyst.plans.logical
+import org.apache.spark.util.{SparkErrorUtils, SparkStringUtils}
+
+/**
+ * Provides [[checkAnswer]] helper for SQL- & DataFrame-API tests.
+ *
+ * TODO: should be moved to sql/api together with SessionQueryTestBase
+ */
+@Experimental
+trait CheckAnswerHelper extends Assertions {
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ protected def checkAnswer(df: => DataFrame, expectedAnswer: Seq[Row]): Unit = {
+
+ val analyzedDF = try df catch {
+ case ae: ExtendedAnalysisException =>
+ if (ae.plan.isDefined) {
+ fail(
+ s"""
+ |Failed to analyze query: $ae
+ |${ae.plan.get}
+ |
+ |${SparkErrorUtils.stackTraceToString(ae)}
+ |""".stripMargin)
+ } else {
+ throw ae
+ }
+ }
+
+ getErrorMessageInCheckAnswer(analyzedDF, expectedAnswer) match {
+ case Some(errorMessage) => fail(errorMessage)
+ case None =>
+ }
+ }
+
+ /*
+ * Note: when moving this to sql/api, implementation should stay in sql/core
+ * (i.e. only have abstract decl in sql/api)
+ */
+ protected def isDfSorted(df: DataFrame): Boolean = {
+ df match {
+ case df: classic.DataFrame =>
+ df.logicalPlan.collectFirst { case s: logical.Sort => s }.nonEmpty
+ case _ =>
+ // isDfSorted should be overriden by connect so that this case can't be reached.
Review Comment:
Thanks! Fixed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "ionagamed (via GitHub)" <gi...@apache.org>.
ionagamed commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3491364459
##########
sql/core/src/test/scala/org/apache/spark/sql/classic/SessionQueryTest.scala:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.classic
+
+import org.apache.spark.sql
+
+/**
+ * Override of [[sql.SessionQueryTest]] that provides [[SparkSession classic.SparkSession]].
+ *
+ * Can be used to declare classic-specific tests:
+ * {{{
+ * class FooSuite extends sql.SessionQueryTest {
+ * // shared classic/connect-agnostic testcases
+ * }
+ *
+ * // no need to extend FooSuite as sql.SessionQueryTest
+ * // already executes shared tests via classic internally.
+ * class FooClassicSuite extends classic.SessionQueryTest {
Review Comment:
In a separate discussion with @fwc we've reached an agreement that the "DAMP, not DRY" way would probably to duplicate the required setup — it's either small enough to duplicate without issues, or big enough that it should be its own separate implementation that is callable by either test.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #56190: [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession
URL: https://github.com/apache/spark/pull/56190
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "ionagamed (via GitHub)" <gi...@apache.org>.
ionagamed commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3441613379
##########
sql/core/src/test/scala/org/apache/spark/sql/classic/SessionQueryTest.scala:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.classic
+
+import org.apache.spark.sql
+
+/**
+ * Override of [[sql.SessionQueryTest]] that provides [[SparkSession classic.SparkSession]].
+ *
+ * Can be used to declare classic-specific tests:
+ * {{{
+ * class FooSuite extends sql.SessionQueryTest {
+ * // shared classic/connect-agnostic testcases
+ * }
+ *
+ * // no need to extend FooSuite as sql.SessionQueryTest
+ * // already executes shared tests via classic internally.
+ * class FooClassicSuite extends classic.SessionQueryTest {
Review Comment:
QQ: What should a test author do when they want to reuse some common utilities from `FooSuite` in their `FooClassicSuite`? `FooBaseSuite` should work fine, but I think it might be worth mentioning the recommended way to handle these situations.
##########
core/src/test/scala/org/apache/spark/CheckErrorHelper.scala:
##########
@@ -0,0 +1,206 @@
+/*
+ * 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
+
+import scala.collection.mutable.ListBuffer
+import scala.jdk.CollectionConverters._
+
+import org.scalatest.Suite
+
+trait CheckErrorHelper { self: Suite =>
+
+ case class ExpectedContext(
+ contextType: QueryContextType,
+ objectType: String,
+ objectName: String,
+ startIndex: Int,
+ stopIndex: Int,
+ fragment: String,
+ callSitePattern: String
+ )
+
+ object ExpectedContext {
+ def apply(fragment: String, start: Int, stop: Int): ExpectedContext = {
+ ExpectedContext("", "", start, stop, fragment)
+ }
+
+ // Check the fragment only. This is only used when the fragment is distinguished within
+ // the query text
+ def apply(fragment: String): ExpectedContext = {
+ ExpectedContext("", "", -1, -1, fragment)
+ }
+
+ def apply(
+ objectType: String,
+ objectName: String,
+ startIndex: Int,
+ stopIndex: Int,
+ fragment: String): ExpectedContext = {
+ new ExpectedContext(QueryContextType.SQL, objectType, objectName, startIndex, stopIndex,
+ fragment, "")
+ }
+
+ def apply(fragment: String, callSitePattern: String): ExpectedContext = {
+ new ExpectedContext(QueryContextType.DataFrame, "", "", -1, -1, fragment, callSitePattern)
+ }
+ }
+
+ /**
+ * Parameter keys that are omitted from comparison when absent from the expected map.
+ * For each error condition, the set lists keys that are removed from the actual
+ * exception parameters before comparison with the expected map.
+ * Test suites may override this to add or change ignorable parameters per condition.
+ */
+ protected def checkErrorIgnorableParameters: Map[String, Set[String]] = Map(
+ "TABLE_OR_VIEW_NOT_FOUND" -> Set("searchPath")
+ )
+
+ /**
+ * Checks an exception with an error condition against expected results.
+ * @param exception The exception to check
+ * @param condition The expected error condition identifying the error
+ * @param sqlState Optional the expected SQLSTATE, not verified if not supplied
+ * @param parameters A map of parameter names and values. The names are as defined
+ * in the error-classes file.
+ * @param matchPVals Optionally treat the parameters value as regular expression pattern.
+ * false if not supplied.
+ */
+ protected def checkError(
+ exception: SparkThrowable,
+ condition: String,
+ sqlState: Option[String] = None,
+ parameters: Map[String, String] = Map.empty,
+ matchPVals: Boolean = false,
+ queryContext: Array[ExpectedContext] = Array.empty): Unit = {
Review Comment:
nit: `queryContext` seems missing from the doc above.
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTestBase.scala:
##########
@@ -0,0 +1,51 @@
+/*
+ * 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
+// 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 QueryCleanupHelper {
+
+ /**
+ * Documents used session so that tests can handle and document session-specific behaviour
+ *
+ * {{{
+ * test(...) {
+ * val df = // query with connect-specific behaviour
+ * if (sessionType == "connect") {
+ * checkError(...)
+ * } else {
+ * checkAnswer(df, ...)
+ * }
+ * }
+ * }}}
+ */
+ def sessionType: String
Review Comment:
QQ: Why is this escape hatch here? The overall approach that this PR seems to be going by is discouraging writing implementation-specific tests, and it seems to me that this goes against that spirit?
It might be worth it to either leave the escape hatch in and document the discouragement; or leave it out altogether.
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/SparkSessionBinder.scala:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.connect
+
+import java.util.UUID
+
+import org.apache.spark.{SparkEnv, SparkFunSuite}
+import org.apache.spark.sql
+import org.apache.spark.sql.classic
+import org.apache.spark.sql.connect.client.SparkConnectClient
+import org.apache.spark.sql.connect.config.Connect
+import org.apache.spark.sql.connect.service.SparkConnectService
+
+/**
+ * Provides a [[SparkSession connect.SparkSession]] backed by an in-process gRPC server.
+ * Extends [[sql.SparkSessionBinder sql.SparkSessionBinder]] (which creates a
+ * [[classic.SparkSession classic.SparkSession]] and SparkContext), then layers a Connect client
+ * session on top by starting the gRPC service in-process.
+ */
+trait SparkSessionBinder extends sql.SparkSessionBinder { self: SparkFunSuite =>
+
+ private var _connectSpark: SparkSession = _
+
+ protected override def spark: SparkSession = _connectSpark
+
+ /** The underlying classic session used by the in-process server. */
+ protected def classicSpark: classic.SparkSession = super.spark.asInstanceOf[classic.SparkSession]
+
+ override protected def beforeAll(): Unit = {
+ super.beforeAll()
+ // Other suites using mocks leave a mess in the global executionManager,
+ // shut it down so that it's cleared before starting server.
+ SparkConnectService.executionManager.shutdown()
+ val prevPort = SparkEnv.get.conf.get(Connect.CONNECT_GRPC_BINDING_PORT)
+ try {
+ // set GRPC_BINDING_PORT to 0 so that the server picks a random, freely available port.
+ SparkEnv.get.conf.set(Connect.CONNECT_GRPC_BINDING_PORT, 0)
+ SparkConnectService.start(classicSpark.sparkContext)
+ } finally {
+ SparkEnv.get.conf.set(Connect.CONNECT_GRPC_BINDING_PORT, prevPort)
+ }
+ }
+
+ override def beforeEach(): Unit = {
+ val client = SparkConnectClient
Review Comment:
QQ: Why are we creating a fresh `client` for each test? The current `SharedSparkSession` is using the suite-level hooks to provide a single shared session for the whole suite.
##########
sql/core/src/test/scala/org/apache/spark/sql/CheckAnswerHelper.scala:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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
+
+import java.util.TimeZone
+
+import org.scalatest.Assertions
+
+import org.apache.spark.annotation.Experimental
+import org.apache.spark.sql.catalyst.plans.logical
+import org.apache.spark.util.{SparkErrorUtils, SparkStringUtils}
+
+/**
+ * Provides [[checkAnswer]] helper for SQL- & DataFrame-API tests.
+ *
+ * TODO: should be moved to sql/api together with SessionQueryTestBase
+ */
+@Experimental
+trait CheckAnswerHelper extends Assertions {
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ protected def checkAnswer(df: => DataFrame, expectedAnswer: Seq[Row]): Unit = {
+ getErrorMessageInCheckAnswer(df, expectedAnswer) match {
+ case Some(errorMessage) => fail(errorMessage)
+ case None =>
+ }
+ }
+
+ /*
+ * Note: when moving this to sql/api, implementation should stay in sql/core
+ * (i.e. only have abstract decl in sql/api)
+ */
+ protected def isDfSorted(df: DataFrame): Boolean = {
+ df match {
+ case df: classic.DataFrame =>
+ df.logicalPlan.collectFirst { case s: logical.Sort => s }.nonEmpty
+ case _ => throw new RuntimeException(s"Cannot determine whether df is sorted: $df")
+ }
+ }
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ * If there was exception during the execution or the contents of the DataFrame does not
Review Comment:
nit:
```suggestion
* If there was an exception during the execution or the contents of the DataFrame do not
```
##########
sql/core/src/test/scala/org/apache/spark/sql/CheckAnswerHelper.scala:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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
+
+import java.util.TimeZone
+
+import org.scalatest.Assertions
+
+import org.apache.spark.annotation.Experimental
+import org.apache.spark.sql.catalyst.plans.logical
+import org.apache.spark.util.{SparkErrorUtils, SparkStringUtils}
+
+/**
+ * Provides [[checkAnswer]] helper for SQL- & DataFrame-API tests.
+ *
+ * TODO: should be moved to sql/api together with SessionQueryTestBase
+ */
+@Experimental
+trait CheckAnswerHelper extends Assertions {
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ protected def checkAnswer(df: => DataFrame, expectedAnswer: Seq[Row]): Unit = {
+ getErrorMessageInCheckAnswer(df, expectedAnswer) match {
+ case Some(errorMessage) => fail(errorMessage)
+ case None =>
+ }
+ }
+
+ /*
+ * Note: when moving this to sql/api, implementation should stay in sql/core
+ * (i.e. only have abstract decl in sql/api)
+ */
+ protected def isDfSorted(df: DataFrame): Boolean = {
+ df match {
+ case df: classic.DataFrame =>
+ df.logicalPlan.collectFirst { case s: logical.Sort => s }.nonEmpty
+ case _ => throw new RuntimeException(s"Cannot determine whether df is sorted: $df")
+ }
+ }
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ * If there was exception during the execution or the contents of the DataFrame does not
+ * match the expected result, an error message will be returned. Otherwise, a None will
+ * be returned.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ private def getErrorMessageInCheckAnswer(
+ df: DataFrame,
+ expectedAnswer: Seq[Row]): Option[String] = {
+ val sparkAnswer = try df.collect().toSeq catch {
+ case e: Exception =>
+ val errorMessage =
+ s"""
+ |Exception thrown while executing query:
+ |${df.queryExecution}
+ |== Exception ==
+ |$e
+ |${SparkErrorUtils.stackTraceToString(e)}
+ """.stripMargin
+ return Some(errorMessage)
+ }
+
+ sameRows(expectedAnswer, sparkAnswer, isDfSorted(df)).map { results =>
+ s"""
+ |Results do not match for query:
+ |Timezone: ${TimeZone.getDefault}
+ |Timezone Env: ${sys.env.getOrElse("TZ", "")}
+ |
+ |${df.queryExecution}
+ |== Results ==
+ |$results
+ """.stripMargin
+ }
+ }
+
+ private def prepareAnswer(answer: Seq[Row], isSorted: Boolean): Seq[Row] = {
+ // Converts data to types that we can do equality comparison using Scala collections.
+ // For BigDecimal type, the Scala type has a better definition of equality test (similar to
+ // Java's java.math.BigDecimal.compareTo).
+ // For binary arrays, we convert it to Seq to avoid of calling java.util.Arrays.equals for
+ // equality test.
+ val converted: Seq[Row] = answer.map(prepareRow)
+ if (!isSorted) converted.sortBy(_.toString()) else converted
+ }
+
+ // We need to call prepareRow recursively to handle schemas with struct types.
+ private def prepareRow(row: Row): Row = {
+ Row.fromSeq(row.toSeq.map {
+ case null => null
+ case bd: java.math.BigDecimal => BigDecimal(bd)
+ // Equality of WrappedArray differs for AnyVal and AnyRef in Scala 2.12.2+
+ case seq: Seq[_] => seq.map {
+ case b: java.lang.Byte => b.byteValue
+ case s: java.lang.Short => s.shortValue
+ case i: java.lang.Integer => i.intValue
+ case l: java.lang.Long => l.longValue
+ case f: java.lang.Float => f.floatValue
+ case d: java.lang.Double => d.doubleValue
+ case x => x
+ }
+ // Convert array to Seq for easy equality check.
+ case b: Array[_] => b.toSeq
+ case r: Row => prepareRow(r)
+ // SPARK-51349: "null" and null had the same precedence in sorting
+ case "null" => "__null_string__"
+ case o => o
+ })
+ }
+
+ private def genError(
+ expectedAnswer: Seq[Row],
+ sparkAnswer: Seq[Row],
+ isSorted: Boolean = false): String = {
+ val getRowType: Option[Row] => String = row =>
+ row.map(row =>
+ if (row.schema == null) {
+ "struct<>"
+ } else {
+ s"${row.schema.catalogString}"
+ }).getOrElse("struct<>")
Review Comment:
nit: this might read better?
```suggestion
row.map(r => Option(r.schema))
.map(s => s"${s.catalogString}")
.getOrElse("struct<>")
```
##########
sql/core/src/test/scala/org/apache/spark/sql/CheckAnswerHelper.scala:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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
+
+import java.util.TimeZone
+
+import org.scalatest.Assertions
+
+import org.apache.spark.annotation.Experimental
+import org.apache.spark.sql.catalyst.plans.logical
+import org.apache.spark.util.{SparkErrorUtils, SparkStringUtils}
+
+/**
+ * Provides [[checkAnswer]] helper for SQL- & DataFrame-API tests.
+ *
+ * TODO: should be moved to sql/api together with SessionQueryTestBase
+ */
+@Experimental
+trait CheckAnswerHelper extends Assertions {
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ protected def checkAnswer(df: => DataFrame, expectedAnswer: Seq[Row]): Unit = {
+ getErrorMessageInCheckAnswer(df, expectedAnswer) match {
+ case Some(errorMessage) => fail(errorMessage)
+ case None =>
+ }
+ }
+
+ /*
+ * Note: when moving this to sql/api, implementation should stay in sql/core
+ * (i.e. only have abstract decl in sql/api)
+ */
+ protected def isDfSorted(df: DataFrame): Boolean = {
+ df match {
+ case df: classic.DataFrame =>
+ df.logicalPlan.collectFirst { case s: logical.Sort => s }.nonEmpty
+ case _ => throw new RuntimeException(s"Cannot determine whether df is sorted: $df")
+ }
+ }
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ * If there was exception during the execution or the contents of the DataFrame does not
+ * match the expected result, an error message will be returned. Otherwise, a None will
+ * be returned.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ private def getErrorMessageInCheckAnswer(
+ df: DataFrame,
+ expectedAnswer: Seq[Row]): Option[String] = {
+ val sparkAnswer = try df.collect().toSeq catch {
+ case e: Exception =>
+ val errorMessage =
+ s"""
+ |Exception thrown while executing query:
+ |${df.queryExecution}
+ |== Exception ==
+ |$e
+ |${SparkErrorUtils.stackTraceToString(e)}
+ """.stripMargin
+ return Some(errorMessage)
+ }
+
+ sameRows(expectedAnswer, sparkAnswer, isDfSorted(df)).map { results =>
+ s"""
+ |Results do not match for query:
+ |Timezone: ${TimeZone.getDefault}
+ |Timezone Env: ${sys.env.getOrElse("TZ", "")}
+ |
+ |${df.queryExecution}
+ |== Results ==
+ |$results
+ """.stripMargin
+ }
+ }
+
+ private def prepareAnswer(answer: Seq[Row], isSorted: Boolean): Seq[Row] = {
+ // Converts data to types that we can do equality comparison using Scala collections.
+ // For BigDecimal type, the Scala type has a better definition of equality test (similar to
+ // Java's java.math.BigDecimal.compareTo).
+ // For binary arrays, we convert it to Seq to avoid of calling java.util.Arrays.equals for
+ // equality test.
+ val converted: Seq[Row] = answer.map(prepareRow)
+ if (!isSorted) converted.sortBy(_.toString()) else converted
+ }
+
+ // We need to call prepareRow recursively to handle schemas with struct types.
+ private def prepareRow(row: Row): Row = {
+ Row.fromSeq(row.toSeq.map {
+ case null => null
+ case bd: java.math.BigDecimal => BigDecimal(bd)
+ // Equality of WrappedArray differs for AnyVal and AnyRef in Scala 2.12.2+
+ case seq: Seq[_] => seq.map {
+ case b: java.lang.Byte => b.byteValue
+ case s: java.lang.Short => s.shortValue
+ case i: java.lang.Integer => i.intValue
+ case l: java.lang.Long => l.longValue
+ case f: java.lang.Float => f.floatValue
+ case d: java.lang.Double => d.doubleValue
+ case x => x
+ }
+ // Convert array to Seq for easy equality check.
+ case b: Array[_] => b.toSeq
+ case r: Row => prepareRow(r)
+ // SPARK-51349: "null" and null had the same precedence in sorting
+ case "null" => "__null_string__"
+ case o => o
+ })
+ }
+
+ private def genError(
+ expectedAnswer: Seq[Row],
+ sparkAnswer: Seq[Row],
+ isSorted: Boolean = false): String = {
+ val getRowType: Option[Row] => String = row =>
+ row.map(row =>
+ if (row.schema == null) {
+ "struct<>"
+ } else {
+ s"${row.schema.catalogString}"
+ }).getOrElse("struct<>")
+
+ s"""
+ |== Results ==
+ |${
+ SparkStringUtils.sideBySide(
+ s"== Correct Answer - ${expectedAnswer.size} ==" +:
+ getRowType(expectedAnswer.headOption) +:
+ prepareAnswer(expectedAnswer, isSorted).map(_.toString()),
+ s"== Spark Answer - ${sparkAnswer.size} ==" +:
+ getRowType(sparkAnswer.headOption) +:
+ prepareAnswer(sparkAnswer, isSorted).map(_.toString())).mkString("\n")
+ }
+ """.stripMargin
+ }
+
+ private def compare(obj1: Any, obj2: Any): Boolean = (obj1, obj2) match {
+ case (null, null) => true
+ case (null, _) => false
+ case (_, null) => false
+ case (a: Array[_], b: Array[_]) =>
Review Comment:
QQ: Aren't `Array`s always converted to `Seq`s above?
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTest.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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
+
+import org.apache.spark.SparkFunSuite
+
+/**
+ * Provides connect-compatible test utils to write suites that have 'connect variants':
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
Review Comment:
nit: same as my comment from above, consider adding guidance on how to handle `FooConnectSuite` needing utils from `FooSuite`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3453822699
##########
sql/core/src/test/scala/org/apache/spark/sql/CheckAnswerHelper.scala:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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
+
+import java.util.TimeZone
+
+import org.scalatest.Assertions
+
+import org.apache.spark.annotation.Experimental
+import org.apache.spark.sql.catalyst.plans.logical
+import org.apache.spark.util.{SparkErrorUtils, SparkStringUtils}
+
+/**
+ * Provides [[checkAnswer]] helper for SQL- & DataFrame-API tests.
+ *
+ * TODO: should be moved to sql/api together with SessionQueryTestBase
+ */
+@Experimental
+trait CheckAnswerHelper extends Assertions {
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ protected def checkAnswer(df: => DataFrame, expectedAnswer: Seq[Row]): Unit = {
+ getErrorMessageInCheckAnswer(df, expectedAnswer) match {
+ case Some(errorMessage) => fail(errorMessage)
+ case None =>
+ }
+ }
+
+ /*
+ * Note: when moving this to sql/api, implementation should stay in sql/core
+ * (i.e. only have abstract decl in sql/api)
+ */
+ protected def isDfSorted(df: DataFrame): Boolean = {
+ df match {
+ case df: classic.DataFrame =>
+ df.logicalPlan.collectFirst { case s: logical.Sort => s }.nonEmpty
+ case _ => throw new RuntimeException(s"Cannot determine whether df is sorted: $df")
+ }
+ }
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ * If there was exception during the execution or the contents of the DataFrame does not
+ * match the expected result, an error message will be returned. Otherwise, a None will
+ * be returned.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ private def getErrorMessageInCheckAnswer(
+ df: DataFrame,
+ expectedAnswer: Seq[Row]): Option[String] = {
+ val sparkAnswer = try df.collect().toSeq catch {
+ case e: Exception =>
+ val errorMessage =
+ s"""
+ |Exception thrown while executing query:
+ |${df.queryExecution}
+ |== Exception ==
+ |$e
+ |${SparkErrorUtils.stackTraceToString(e)}
+ """.stripMargin
+ return Some(errorMessage)
+ }
+
+ sameRows(expectedAnswer, sparkAnswer, isDfSorted(df)).map { results =>
+ s"""
+ |Results do not match for query:
+ |Timezone: ${TimeZone.getDefault}
+ |Timezone Env: ${sys.env.getOrElse("TZ", "")}
+ |
+ |${df.queryExecution}
+ |== Results ==
+ |$results
+ """.stripMargin
+ }
+ }
+
+ private def prepareAnswer(answer: Seq[Row], isSorted: Boolean): Seq[Row] = {
+ // Converts data to types that we can do equality comparison using Scala collections.
+ // For BigDecimal type, the Scala type has a better definition of equality test (similar to
+ // Java's java.math.BigDecimal.compareTo).
+ // For binary arrays, we convert it to Seq to avoid of calling java.util.Arrays.equals for
+ // equality test.
+ val converted: Seq[Row] = answer.map(prepareRow)
+ if (!isSorted) converted.sortBy(_.toString()) else converted
+ }
+
+ // We need to call prepareRow recursively to handle schemas with struct types.
+ private def prepareRow(row: Row): Row = {
+ Row.fromSeq(row.toSeq.map {
+ case null => null
+ case bd: java.math.BigDecimal => BigDecimal(bd)
+ // Equality of WrappedArray differs for AnyVal and AnyRef in Scala 2.12.2+
+ case seq: Seq[_] => seq.map {
+ case b: java.lang.Byte => b.byteValue
+ case s: java.lang.Short => s.shortValue
+ case i: java.lang.Integer => i.intValue
+ case l: java.lang.Long => l.longValue
+ case f: java.lang.Float => f.floatValue
+ case d: java.lang.Double => d.doubleValue
+ case x => x
+ }
+ // Convert array to Seq for easy equality check.
+ case b: Array[_] => b.toSeq
+ case r: Row => prepareRow(r)
+ // SPARK-51349: "null" and null had the same precedence in sorting
+ case "null" => "__null_string__"
+ case o => o
+ })
+ }
+
+ private def genError(
+ expectedAnswer: Seq[Row],
+ sparkAnswer: Seq[Row],
+ isSorted: Boolean = false): String = {
+ val getRowType: Option[Row] => String = row =>
+ row.map(row =>
+ if (row.schema == null) {
+ "struct<>"
+ } else {
+ s"${row.schema.catalogString}"
+ }).getOrElse("struct<>")
+
+ s"""
+ |== Results ==
+ |${
+ SparkStringUtils.sideBySide(
+ s"== Correct Answer - ${expectedAnswer.size} ==" +:
+ getRowType(expectedAnswer.headOption) +:
+ prepareAnswer(expectedAnswer, isSorted).map(_.toString()),
+ s"== Spark Answer - ${sparkAnswer.size} ==" +:
+ getRowType(sparkAnswer.headOption) +:
+ prepareAnswer(sparkAnswer, isSorted).map(_.toString())).mkString("\n")
+ }
+ """.stripMargin
+ }
+
+ private def compare(obj1: Any, obj2: Any): Boolean = (obj1, obj2) match {
+ case (null, null) => true
+ case (null, _) => false
+ case (_, null) => false
+ case (a: Array[_], b: Array[_]) =>
Review Comment:
This code is copy-pasted from `QueryTest`, :shrug:
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3453972320
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -823,21 +789,39 @@ trait QueryTest extends SparkFunSuite with QueryTestBase {
}
}
-object QueryTest extends Assertions {
+@deprecated("superseded by CheckAnswerHelper", since = "4.2")
+object QueryTest extends CheckAnswerHelper {
Review Comment:
> the kept methods could delegate to the helper's internals so the logic exists once
But then we couldn't make the helpers private in `CheckAnswerHelper`, right?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SparkSessionBinder to replace SharedSparkSession [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3392033486
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -156,7 +157,7 @@ trait QueryTestBase
* @param df the [[DataFrame]] to be executed
* @param expectedAnswer the expected result in a [[Seq]] of [[Row]]s.
*/
- protected def checkAnswer(df: => DataFrame, expectedAnswer: Seq[Row]): Unit = {
+ override protected def checkAnswer(df: => DataFrame, expectedAnswer: Seq[Row]): Unit = {
Review Comment:
This override is why the whole connect DSv2 suite fails with `UNSUPPORTED_CONNECT_FEATURE.DATASET_QUERY_EXECUTION`. It wins the linearization over the connect-compatible `CheckAnswerHelper.checkAnswer`, and it routes every call through `assertEmptyMissingInput` (`df.queryExecution`) and `QueryTest.checkAnswer` (`df.materializedRdd`) — both classic-only. Any suite with `QueryTestBase` in its linearization gets this path, including the DSv2 traits (`QueryTest with SessionQueryTestBase`), so mixing in `connect.SparkSessionBinder` can't make an existing suite Connect-runnable. This confirms my earlier comment on the since-deleted `connect/QueryTest.scala`: the helpers reaching classic-only APIs are the real blocker.
I'd fix it here rather than in each suite: delegate to `super.checkAnswer` (the helper) and apply the classic-only extras — `assertEmptyMissingInput` and the `checkToRDD` pass — only when `df` is a `classic.DataFrame`.
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/DataSourceV2DataFrameConnectSuite.scala:
##########
@@ -34,7 +34,7 @@ import org.apache.spark.sql.connector.catalog.{CachingInMemoryTableCatalog, InMe
* this class only provides the Connect-specific session, catalog access, and result comparison.
*/
class DataSourceV2DataFrameConnectSuite
- extends SparkConnectServerTest
+ extends SessionQueryTest
Review Comment:
Switching from `SparkConnectServerTest` to a suite-wide session drops the per-test isolation the old base provided (`afterEach` -> `invalidateAllSessions()`), and that's what the `TABLE_OR_VIEW_ALREADY_EXISTS` CI failures are: server-side catalog state now leaks across tests. The classic peer compensates with `after { cachingcat.clearCache(); catalogManager.reset() }` (`DataSourceV2DataFrameSuite.scala:86-89`); this suite has no counterpart. I'd recreate the client session per test and invalidate server sessions in `afterEach` — that restores the isolation the caching-catalog tests are written against; failing that, add the same per-test catalog cleanup the classic suite uses.
##########
sql/core/src/test/scala/org/apache/spark/sql/ExampleSuite.scala:
##########
@@ -0,0 +1,47 @@
+/*
+ * 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
+
+class ExampleSuite extends SessionQueryTest {
+
+ test("replaceWhere with partitioned table preserves all partitions") {
+ withTable("foo") {
+ val data = Seq(
+ (1, "Alice", 29),
+ (2, "Bob", 35),
+ (3, "Charlie", 23),
+ )
+
+ val df = spark.createDataFrame(data).toDF("id", "name", "age")
+
+ df.write.partitionBy("age").format("delta").saveAsTable("foo")
Review Comment:
`delta` isn't a data source in Apache Spark — this suite fails every CI run with `DATA_SOURCE_NOT_FOUND` (and `replaceWhere` is Delta-specific too). This looks like a local experiment that got committed. If you want a template suite demonstrating the pattern, rewrite it against a built-in source or the in-memory v2 catalog with assertions that hold here; otherwise drop both `ExampleSuite` and `ExampleConnectSuite`.
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -823,21 +789,39 @@ trait QueryTest extends SparkFunSuite with QueryTestBase {
}
}
-object QueryTest extends Assertions {
+@deprecated("superseded by CheckAnswerHelper", since = "4.2")
+object QueryTest extends CheckAnswerHelper {
Review Comment:
Now that the object extends `CheckAnswerHelper`, it carries two parallel copies of the comparison logic — the helper's private ones and the public `getErrorMessageInCheckAnswer`/`prepareAnswer`/`sameRows` kept below. The public surface is still called (e.g. `AggregationQuerySuite.scala:1089`), so it can't be deleted, but the kept methods could delegate to the helper's internals so the logic exists once. Worth doing while the file is being restructured — drift between the two copies will be invisible.
##########
sql/core/src/test/scala/org/apache/spark/sql/SparkSessionBinder.scala:
##########
@@ -0,0 +1,178 @@
+/*
+ * 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
+
+import scala.concurrent.duration._
+
+import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach, Suite}
+import org.scalatest.concurrent.Eventually
+
+import org.apache.spark.{DebugFilesystem, SparkConf, SparkFunSuite}
+import org.apache.spark.internal.config.UNSAFE_EXCEPTION_ON_MEMORY_LEAK
+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.test.TestSparkSession
+
+/**
+ * Provides a [[spark]] implementation by creating a [[classic.SparkSession]].
+ *
+ * counterpart to [[SparkSessionProvider]], used in [[org.apache.spark.sql.test.SharedSparkSession]]
+ */
+trait SparkSessionBinder extends SparkSessionBinderBase { self: SparkFunSuite =>
+
+ /**
+ * Suites extending this trait are sharing resources (e.g. SparkSession) in their
+ * tests. This 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()
+ }
+ }
+}
+
+trait SparkSessionBinderBase
Review Comment:
The Base/Binder split is doing real work, but nothing documents it, so it reads as permanent vocabulary when it's transitional. As far as I can trace, the split exists because (a) the thread audit needs `self: SparkFunSuite` while the FunSpec/WordSpec/FlatSpec suites reachable via `SharedSparkSessionBase` need `self: Suite`, and (b) the deprecated `SharedSparkSession` aliases recompose the binder with the old `QueryTest` API. Once those migrate, the binder content can merge into `SessionQueryTest`, with the per-env variants overriding `spark`/`createSparkSession` directly. Please add a scaladoc paragraph stating each layer's role, and a TODO recording that cleanup plan — this PR is establishing the test-infra vocabulary people will copy, and the layering rationale shouldn't have to be reverse-engineered from `GenericFunSpecSuite`.
##########
sql/core/src/test/scala/org/apache/spark/sql/CheckAnswerHelper.scala:
##########
@@ -0,0 +1,183 @@
+/*
+ * 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
+
+import java.util.TimeZone
+
+import org.scalatest.Assertions
+
+import org.apache.spark.sql.catalyst.plans.logical
+import org.apache.spark.util.{SparkErrorUtils, SparkStringUtils}
+
+trait CheckAnswerHelper extends Assertions {
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ protected def checkAnswer(df: => DataFrame, expectedAnswer: Seq[Row]): Unit = {
+ getErrorMessageInCheckAnswer(df, expectedAnswer) match {
+ case Some(errorMessage) => fail(errorMessage)
+ case None =>
+ }
+ }
+
+ protected def isDfSorted(df: DataFrame): Boolean = {
+ df match {
+ case df: classic.DataFrame =>
+ df.logicalPlan.collectFirst { case s: logical.Sort => s }.nonEmpty
+ case _ => throw new RuntimeException(s"Cannot determine whether df is sorted: $df")
+ }
+ }
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ * If there was exception during the execution or the contents of the DataFrame does not
+ * match the expected result, an error message will be returned. Otherwise, a None will
+ * be returned.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ private def getErrorMessageInCheckAnswer(
+ df: DataFrame,
+ expectedAnswer: Seq[Row]): Option[String] = {
+ val sparkAnswer = try df.collect().toSeq catch {
+ case e: Exception =>
+ val errorMessage =
+ s"""
+ |Exception thrown while executing query:
+ |${df.queryExecution}
Review Comment:
Both failure messages here interpolate `${df.queryExecution}`, which throws `UNSUPPORTED_CONNECT_FEATURE` on Connect client DataFrames — so on Connect, a *failing* `checkAnswer` reports the unsupported-API error instead of the row mismatch. Since this helper exists to be connect-compatible, guard the interpolation: include the plan only for `classic.DataFrame` and fall back to `df.toString` otherwise. (Same on line 81.)
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/SessionQueryTest.scala:
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.connect
+
+import org.apache.spark.sql
+
+/**
+ * Overrides test utils to implement 'connect variants' of suites declared in sql/core:
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
+ *
+ * // in sql/connect
+ * FooConnectSuite extends connect.SessionQueryTest
+ * }}}
+ *
+ * This trait overrides [[spark]] to use a [[SparkSession connect.SparkSession]], which executes
+ * via the gRPC API using an in-process connect server.
+ */
+trait SessionQueryTest extends sql.SessionQueryTest with SparkSessionBinder {
+ final override def isDfSorted(df: sql.DataFrame): Boolean = false // TODO
Review Comment:
Returning `false` makes every comparison order-insensitive on Connect — an `ORDER BY` test that passes on classic can no longer fail on wrong ordering in its Connect variant. Worth a scaladoc note on the trait spelling that out, and a JIRA reference on the TODO, so suite authors know ordering assertions are silently weakened.
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/DataSourceV2DataFrameConnectSuite.scala:
##########
@@ -53,45 +53,15 @@ class DataSourceV2DataFrameConnectSuite
.set("spark.sql.catalog.nullbothidscat.copyOnLoad", "true")
override protected def testPrefix: String = "[connect] "
- override protected def isConnect: Boolean = true
-
- override protected def withTestSession(fn: SparkSession => Unit): Unit =
- withSession(fn)
-
- // Cannot use QueryTest.checkAnswer directly because it accesses df.logicalPlan,
- // df.queryExecution, and df.materializedRdd, which are not available on Connect *client*
- // DataFrames (they throw ConnectClientUnsupportedErrors). Note: checkAnswer IS usable from
- // Connect server tests that operate on classic server-side DataFrames, but in this suite
- // `df` is a Connect client DataFrame returned by session.table() / session.sql().
- // Instead, collect the rows and delegate to QueryTest.sameRows, which is the same
- // value-based, order-agnostic comparison that checkAnswer uses internally.
- override protected def checkRows(df: => DataFrame, expected: Seq[Row]): Unit =
- QueryTest.sameRows(expected, df.collect().toSeq).foreach(msg => fail(msg))
override protected def getTableCatalog[C <: TableCatalog: ClassTag](
session: SparkSession,
catalogName: String): C = {
- val serverSession = getServerSession(session)
- val catalog = serverSession.sessionState.catalogManager.catalog(catalogName)
+ val catalog = classicSpark.sessionState.catalogManager.catalog(catalogName)
Review Comment:
`classicSpark` is not the session these queries run in. Connect server sessions are created via `newSession()` (`SparkConnectSessionManager.newIsolatedSession`), so they have their own `CatalogManager` and instantiate their own catalog plugin objects — `externalAppend` against this instance mutates state the Connect queries never read. The old implementation used `getServerSession(session)` for exactly this reason. I'd port that accessor onto `connect.SparkSessionBinder` and use it here. The `classicSpark` scaladoc ("The underlying classic session used by the in-process server") needs the same fix — the server never uses that session directly. I should have caught the doc claim last round.
##########
sql/core/src/test/scala/org/apache/spark/sql/SparkSessionBinder.scala:
##########
@@ -0,0 +1,178 @@
+/*
+ * 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
+
+import scala.concurrent.duration._
+
+import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach, Suite}
+import org.scalatest.concurrent.Eventually
+
+import org.apache.spark.{DebugFilesystem, SparkConf, SparkFunSuite}
+import org.apache.spark.internal.config.UNSAFE_EXCEPTION_ON_MEMORY_LEAK
+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.test.TestSparkSession
+
+/**
+ * Provides a [[spark]] implementation by creating a [[classic.SparkSession]].
+ *
+ * counterpart to [[SparkSessionProvider]], used in [[org.apache.spark.sql.test.SharedSparkSession]]
+ */
+trait SparkSessionBinder extends SparkSessionBinderBase { self: SparkFunSuite =>
+
+ /**
+ * Suites extending this trait are sharing resources (e.g. SparkSession) in their
+ * tests. This 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()
+ }
+ }
+}
+
+trait SparkSessionBinderBase
+ extends SparkSessionProvider
+ with BeforeAndAfterEach
+ with BeforeAndAfterAll
+ with Eventually { self: Suite =>
+
+ protected def sparkConf: SparkConf = {
+ val conf = new SparkConf()
+ .set("spark.hadoop.fs.file.impl", classOf[DebugFilesystem].getName)
+ .set(UNSAFE_EXCEPTION_ON_MEMORY_LEAK, true)
+ .set(SQLConf.CODEGEN_FALLBACK.key, "false")
+ .set(SQLConf.CODEGEN_FACTORY_MODE.key, CodegenObjectFactoryMode.CODEGEN_ONLY.toString)
+ // Disable ConvertToLocalRelation for better test coverage. Test cases built on
+ // LocalRelation will exercise the optimization rules better by disabling it as
+ // this rule may potentially block testing of other optimization rules such as
+ // ConstantPropagation etc.
+ .set(SQLConf.OPTIMIZER_EXCLUDED_RULES.key, ConvertToLocalRelation.ruleName)
+ conf.set(
+ StaticSQLConf.WAREHOUSE_PATH,
+ conf.get(StaticSQLConf.WAREHOUSE_PATH) + "/" + getClass.getCanonicalName)
+ conf.set(StaticSQLConf.LOAD_SESSION_EXTENSIONS_FROM_CLASSPATH, false)
+ conf.set(StaticSQLConf.SHUFFLE_EXCHANGE_MAX_THREAD_THRESHOLD,
+ sys.env.getOrElse("SPARK_TEST_SQL_SHUFFLE_EXCHANGE_MAX_THREAD_THRESHOLD",
+ StaticSQLConf.SHUFFLE_EXCHANGE_MAX_THREAD_THRESHOLD.defaultValueString).toInt)
+ conf.set(StaticSQLConf.RESULT_QUERY_STAGE_MAX_THREAD_THRESHOLD,
+ sys.env.getOrElse("SPARK_TEST_SQL_RESULT_QUERY_STAGE_MAX_THREAD_THRESHOLD",
+ StaticSQLConf.RESULT_QUERY_STAGE_MAX_THREAD_THRESHOLD.defaultValueString).toInt)
+ }
+
+ /**
+ * The [[TestSparkSession]] to use for all tests in this suite.
+ *
+ * By default, the underlying [[org.apache.spark.SparkContext]] will be run in local
+ * mode with the default test configurations.
+ */
+ private var _spark: classic.SparkSession = null
+
+ protected override def spark: SparkSession = _spark
+
+ /**
+ * The [[test.TestSQLContext]] to use for all tests in this suite.
Review Comment:
`TestSQLContext` is a `private[sql]` object holding override confs, not this member's type — `sqlContext` returns a plain `SQLContext`. Stale text from the old `SharedSparkSessionBase`.
```suggestion
* The [[SQLContext]] to use for all tests in this suite.
```
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -202,6 +203,7 @@ trait QueryTestBase
* @param expectedAnswer the expected result in a [[Seq]] of [[Row]]s.
* @param absTol the absolute tolerance between actual and expected answers.
*/
+ @deprecated("rarely used")
Review Comment:
Worth a consistent `since = "4.2.0"` and a message naming the replacement (like the `SharedSparkSession` one) — here and on the other two `"rarely used"` annotations, plus the object's `since = "4.2"`.
##########
sql/core/src/test/scala/org/apache/spark/sql/SparkSessionBinder.scala:
##########
@@ -0,0 +1,178 @@
+/*
+ * 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
+
+import scala.concurrent.duration._
+
+import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach, Suite}
+import org.scalatest.concurrent.Eventually
+
+import org.apache.spark.{DebugFilesystem, SparkConf, SparkFunSuite}
+import org.apache.spark.internal.config.UNSAFE_EXCEPTION_ON_MEMORY_LEAK
+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.test.TestSparkSession
+
+/**
+ * Provides a [[spark]] implementation by creating a [[classic.SparkSession]].
+ *
+ * counterpart to [[SparkSessionProvider]], used in [[org.apache.spark.sql.test.SharedSparkSession]]
Review Comment:
```suggestion
* Counterpart to [[SparkSessionProvider]]; used in [[org.apache.spark.sql.test.SharedSparkSession]].
```
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/SessionQueryTest.scala:
##########
@@ -0,0 +1,24 @@
+/*
+ * 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.hive
+
+import org.apache.spark.sql.classic
+
+trait SessionQueryTest extends classic.SessionQueryTest with test.TestHiveSingleton {
Review Comment:
This stacks two competing session lifecycles. `TestHiveSingleton.spark` is an eager `val` (`TestHive.sparkSession`, built at suite construction), but the inherited binder `beforeAll` still runs `initializeSession()` -> `cleanupAnyExistingSession()` + `new TestSparkSession(sparkConf)` — tearing down the active session after TestHive's exists and creating a second, unused one. Nothing in the PR extends this trait, so CI never exercises it. Either add a smoke-test suite or drop the hive variant from this PR; if it stays, the hive binding should override the session-source seam (`createSparkSession`) instead of stacking `TestHiveSingleton` on a live binder lifecycle.
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTestBase.scala:
##########
@@ -0,0 +1,51 @@
+/*
+ * 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
+// 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 QueryCleanupHelper {
+
+ /**
+ * Documents used session so that tests can handle and document session-specific behaviour
+ *
+ * {{{
+ * test(...) {
+ * val df = // query with connect-specific behaviour
+ * if (sessionType = 'connect') {
Review Comment:
The example isn't valid Scala (assignment, and single quotes on a `String`):
```suggestion
* if (sessionType == "connect") {
```
While here: the trait doc on line 27 ("base for fully sql/core independent tests") and the summary on line 37 ("Documents used session so that...") could use a grammar pass too.
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTest.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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
+
+import org.apache.spark.SparkFunSuite
+
+/**
+ * Provides connect-compatible test utils to write suites that have 'connect variants':
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
+ *
+ * // in sql/connect
+ * FooConnectSuite extends connect.SessionQueryTest
+ * }}}
+ *
+ * While this trait internally uses a [[classic.SparkSession]] when executing tests,
+ * it exposed as a [[SparkSession sql.SparkSession]] to allow for overriding on the connect side.
Review Comment:
Grammar:
```suggestion
* it is exposed as a [[SparkSession sql.SparkSession]] to allow for overriding on the connect side.
```
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -823,21 +789,39 @@ trait QueryTest extends SparkFunSuite with QueryTestBase {
}
}
-object QueryTest extends Assertions {
+@deprecated("superseded by CheckAnswerHelper", since = "4.2")
+object QueryTest extends CheckAnswerHelper {
/**
* Runs the plan and makes sure the answer matches the expected result.
*
* @param df the DataFrame to be executed
* @param expectedAnswer the expected result in a Seq of Rows.
* @param checkToRDD whether to verify deserialization to an RDD. This runs the query twice.
Review Comment:
This `@param checkToRDD` now documents the 2-arg overload, which doesn't have the parameter — move it to the 3-arg overload below, or drop it.
##########
sql/core/src/test/scala/org/apache/spark/sql/CheckAnswerHelper.scala:
##########
@@ -0,0 +1,183 @@
+/*
+ * 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
+
+import java.util.TimeZone
+
+import org.scalatest.Assertions
+
+import org.apache.spark.sql.catalyst.plans.logical
+import org.apache.spark.util.{SparkErrorUtils, SparkStringUtils}
+
+trait CheckAnswerHelper extends Assertions {
+
+ /**
+ * Runs the plan and makes sure the answer matches the expected result.
+ *
+ * @param df the DataFrame to be executed
+ * @param expectedAnswer the expected result in a Seq of Rows.
+ */
+ protected def checkAnswer(df: => DataFrame, expectedAnswer: Seq[Row]): Unit = {
Review Comment:
`df` is by-name but is forced before any try (at the `getErrorMessageInCheckAnswer` call), so an analysis-time failure escapes the formatted error path. Either wrap the evaluation like `QueryTestBase.checkAnswer` does with `ExtendedAnalysisException`, or drop the by-name — as written it buys nothing.
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTest.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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
+
+import org.apache.spark.SparkFunSuite
+
+/**
+ * Provides connect-compatible test utils to write suites that have 'connect variants':
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
+ *
+ * // in sql/connect
+ * FooConnectSuite extends connect.SessionQueryTest
+ * }}}
+ *
+ * While this trait internally uses a [[classic.SparkSession]] when executing tests,
+ * it exposed as a [[SparkSession sql.SparkSession]] to allow for overriding on the connect side.
+ *
+ * For classic-specific tests, use [[classic.SessionQueryTest]].
+ */
+trait SessionQueryTest
+ extends SparkFunSuite
+ with SessionQueryTestBase
+ with SparkSessionBinder {
+
+ override def sessionType: String = "classic"
Review Comment:
Missing newline at EOF — scalastyle's `NewLineAtEofChecker` is error-level, and this is failing the linter job.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3461768210
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -1223,4 +1212,19 @@ class QueryTestSuite extends test.SharedSparkSession {
"from range(2)"),
Seq(Row(Row(null)), Row(Row("null"))))
}
+
+ test("checkAnswer demands correct result order for ordered queries") {
+ val e = intercept[org.scalatest.exceptions.TestFailedException] {
+ checkAnswer(
+ sql("SELECT col1 FROM VALUE 1, 2, 1, 3 ORDER BY col 1"),
Review Comment:
`VALUE` isn't valid inline-table syntax — it's `VALUES` — and `col 1` (with a space) isn't a valid column ref. This throws `ParseException`, so the `intercept[TestFailedException]` never matches and the test fails; it's the root cause of the red CI (and `QueryTestWithConnectSuite` inherits and cascades on it). The same `VALUE` typo is on line 1227.
```suggestion
sql("SELECT col1 FROM VALUES 1, 2, 1, 3 ORDER BY col1"),
```
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/SessionQueryTest.scala:
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.connect
+
+import org.apache.spark.sql
+
+/**
+ * Overrides test utils to implement 'connect variants' of suites declared in sql/core:
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
+ *
+ * // in sql/connect
+ * FooConnectSuite extends FooSuite with connect.SessionQueryTest
+ * }}}
+ *
+ * This trait overrides [[spark]] to use a [[SparkSession connect.SparkSession]], which executes
+ * via the gRPC API using an in-process connect server.
+ */
+trait SessionQueryTest extends sql.SessionQueryTest with SparkSessionBinder {
+
+ /**
+ * TODO add required test-only API to Spark Connect
+ * This method is used by [[checkAnswer]] internally but cannot yet be implemented in connect.
+ * Thus we always return `false` for now.
+ */
+ override def isDfSorted(df: sql.DataFrame): Boolean = df match {
+ case df: DataFrame => df.explainString(true).contains("Sort")
+ case df => super.isDfSorted(df)
+ }
Review Comment:
**(blocking)** This `match` block is mis-indented (`case` at 6 spaces, closing `}` at 4) and fails the scalafmt check, which is enforced on `sql/connect/*` (`dev/lint-scala`, maxColumn 98) — that's why the "Scala linter" job is red. Reformat:
```suggestion
override def isDfSorted(df: sql.DataFrame): Boolean = df match {
case df: DataFrame => df.explainString(true).contains("Sort")
case df => super.isDfSorted(df)
}
```
**(non-blocking, question)** `explainString(true)` is the EXTENDED plan, so substring `"Sort"` also matches physical operators like `SortMergeJoin`/`SortAggregate`. An *unordered* query that happens to plan a sort-based operator would then be treated as order-sensitive and could fail spuriously — classic detects a logical `Sort` node instead. Worth matching the analyzed logical plan's `Sort`, or documenting the limitation. (This is the good direction vs. the old always-`false`, just imprecise.)
**(nit)** The scaladoc just above (line 39) still says "Thus we always return `false` for now", which no longer matches this implementation.
##########
sql/core/src/test/scala/org/apache/spark/sql/ExampleSessionAgnosticSuite.scala:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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
+
+import org.apache.spark.SparkConf
+import org.apache.spark.sql.connector.catalog.InMemoryPartitionTableCatalog
+
+/**
+ * Example for
Review Comment:
Incomplete sentence.
```suggestion
* Example of a classic/connect-agnostic test suite.
```
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -1223,4 +1212,19 @@ class QueryTestSuite extends test.SharedSparkSession {
"from range(2)"),
Seq(Row(Row(null)), Row(Row("null"))))
}
+
+ test("checkAnswer demands correct result order for ordered queries") {
+ val e = intercept[org.scalatest.exceptions.TestFailedException] {
+ checkAnswer(
+ sql("SELECT col1 FROM VALUE 1, 2, 1, 3 ORDER BY col 1"),
+ Seq(Row(3), Row(1), Row(1), Row(2)))
+ }
+ assert(e.getMessage().contains("Results do not match for query"))
+ }
+
+ test("checkAnswer ignores result order for unordered queries") {
+ checkAnswer(
+ sql("SELECT col1 FROM VALUE 1, 2, 1, 3"),
Review Comment:
Same `VALUE` -> `VALUES` fix here.
```suggestion
sql("SELECT col1 FROM VALUES 1, 2, 1, 3"),
```
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/QueryTestWithConnectSuite.scala:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.connect
+
+import org.apache.spark.sql.QueryTestSuite
+
+/**
+ * Runs [[QueryTestSuite]] tests through a Connect session.
+ *
+ * This validates the `FooSuite with connect.SparkSessionBinder` pattern: the existing
Review Comment:
The class mixes in `connect.SessionQueryTest` (the pattern the PR recommends), not the bare `connect.SparkSessionBinder` — update the doc to match what the code does:
```suggestion
* This validates the `FooSuite with connect.SessionQueryTest` pattern: the existing
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3457815865
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTest.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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
+
+import org.apache.spark.SparkFunSuite
+
+/**
+ * Provides connect-compatible test utils to write suites that have 'connect variants':
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
+ *
+ * // in sql/connect
+ * FooConnectSuite extends connect.SessionQueryTest
+ * }}}
+ *
+ * While this trait internally uses a [[classic.SparkSession]] when executing tests,
+ * it exposed as a [[SparkSession sql.SparkSession]] to allow for overriding on the connect side.
+ *
+ * For classic-specific tests, use [[classic.SessionQueryTest]].
+ */
+trait SessionQueryTest
+ extends SparkFunSuite
+ with SessionQueryTestBase
+ with SparkSessionBinder {
+
+ override def sessionType: String = "classic"
Review Comment:
I added `sessionType` since this pattern is already used in the classic/connect-agnostic tests here: https://github.com/apache/spark/blob/d76d7f5f6367518b4cb96b796a2c6ac83ec8303a/sql/core/src/test/scala/org/apache/spark/sql/connector/DSv2IncrementallyConstructedQueryTests.scala#L201-L232
> This seems to go against the spirit of writing session-agnostic tests.
AkShUaLLy, diverging classic-/connect-semantics go against the spirit of writing session-agnostic tests.
Having a `sessionType` here is a coping mechanism, and IMHO a good one.
> The suggested test might be implemented cleaner as [classic- and connect-specific test, potentially with shared outlined logic]
I strongly disagree here:
When writing a test to document diverging classic/connect behaviour, that should be _a single testcase_. Your example would generally be in two different files, in two different modules (`sql/core` and `sql/connect/server`). Ofc it is generally possible to view two files side-by-side, but it still requires the linking of 'oh yeah, the second half of this test is somewhere over there'.
(And outlining shared logic into `bar` violates my understanding of [DAMP, not DRY](https://abseil.io/resources/swe-book/html/ch12.html#tests_and_code_sharing_dampcomma_not_dr).)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "johanl-db (via GitHub)" <gi...@apache.org>.
johanl-db commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3453237011
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -202,6 +203,7 @@ trait QueryTestBase
* @param expectedAnswer the expected result in a [[Seq]] of [[Row]]s.
* @param absTol the absolute tolerance between actual and expected answers.
*/
+ @deprecated("rarely used", since = "4.2.0")
Review Comment:
Probably not a good enough reason to deprecate it.
That being said, if it's not really used, it's ok to now provide an equivalent in connect, meaning the few tests using this won't work in connect unless someone put some cycles adding an equivalent method
##########
sql/core/src/test/scala/org/apache/spark/sql/SessionQueryTest.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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
+
+import org.apache.spark.SparkFunSuite
+
+/**
+ * Provides connect-compatible test utils to write suites that have 'connect variants':
+ * {{{
+ * // in sql/core
+ * FooSuite extends SessionQueryTest { test("") { ... } }
+ *
+ * // in sql/connect
+ * FooConnectSuite extends connect.SessionQueryTest
+ * }}}
+ *
+ * While this trait internally uses a [[classic.SparkSession]] when executing tests,
+ * it exposed as a [[SparkSession sql.SparkSession]] to allow for overriding on the connect side.
+ *
+ * For classic-specific tests, use [[classic.SessionQueryTest]].
+ */
+trait SessionQueryTest
+ extends SparkFunSuite
+ with SessionQueryTestBase
+ with SparkSessionBinder {
+
+ override def sessionType: String = "classic"
Review Comment:
What's the goal of this `sessionType`?
SessionQueryTest returns an agnostic session. If callers really want to know what type of session they have, they can use reflection, but it seems to me we should discourage callers poking into the session. If you want a classic session, get an actual one via classic.SessionQueryTest
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/ExampleConnectSuite.scala:
##########
@@ -0,0 +1,22 @@
+/*
+ * 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.connect
+
+import org.apache.spark.sql
+
+class ExampleConnectSuite extends sql.ExampleSuite with SessionQueryTest
Review Comment:
I like the example that Wenchen gave in https://github.com/apache/spark/pull/56190#discussion_r3322066761
Can you colocate the example suites and show how one would write tests that classic only, connect only + shared tests?
A single page example showing everything will also help sharing knowledge of how to use this
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -156,7 +157,7 @@ trait QueryTestBase
* @param df the [[DataFrame]] to be executed
* @param expectedAnswer the expected result in a [[Seq]] of [[Row]]s.
*/
- protected def checkAnswer(df: => DataFrame, expectedAnswer: Seq[Row]): Unit = {
+ override protected def checkAnswer(df: => DataFrame, expectedAnswer: Seq[Row]): Unit = {
Review Comment:
+1, the code should be robust and work with different ordering of traits, otherwise it'll create friction and cause people losing a lot of time trying to figure out what's wrong
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56505][SQL][TESTS] Add SessionQueryTest to replace SharedSparkSession [spark]
Posted by "fwc (via GitHub)" <gi...@apache.org>.
fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3454233682
##########
sql/core/src/test/scala/org/apache/spark/sql/classic/SessionQueryTest.scala:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.classic
+
+import org.apache.spark.sql
+
+/**
+ * Override of [[sql.SessionQueryTest]] that provides [[SparkSession classic.SparkSession]].
+ *
+ * Can be used to declare classic-specific tests:
+ * {{{
+ * class FooSuite extends sql.SessionQueryTest {
+ * // shared classic/connect-agnostic testcases
+ * }
+ *
+ * // no need to extend FooSuite as sql.SessionQueryTest
+ * // already executes shared tests via classic internally.
+ * class FooClassicSuite extends classic.SessionQueryTest {
Review Comment:
Mhhhhhmaybe we could have a `FooClassicSuite` ignore all tests that are already executed in `FooSuite` with some hackery 🤔
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org