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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
