You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/08/17 03:34:47 UTC
[GitHub] [spark] kazuyukitanimura opened a new pull request, #37544: [SPARK-40110][SQL][Tests] Add JDBCWithAQESuite
kazuyukitanimura opened a new pull request, #37544:
URL: https://github.com/apache/spark/pull/37544
### What changes were proposed in this pull request?
This PR proposes to add `JDBCWithAQESuite` i.e. test cases of `JDBCSuite` with AQE (Adaptive Query Execution) enabled.
### Why are the changes needed?
Currently `JDBCSuite` assumes that AQE is always turned off. We should also test with AQE turned on
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Added the AQE version tests along with the non AQE version
--
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
[GitHub] [spark] sunchao commented on pull request #37544: [SPARK-40110][SQL][TESTS] Add JDBCWithAQESuite
Posted by GitBox <gi...@apache.org>.
sunchao commented on PR #37544:
URL: https://github.com/apache/spark/pull/37544#issuecomment-1218559310
Committed to master branch, thanks!
--
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
[GitHub] [spark] kazuyukitanimura commented on pull request #37544: [SPARK-40110][SQL][Tests] Add JDBCWithAQESuite
Posted by GitBox <gi...@apache.org>.
kazuyukitanimura commented on PR #37544:
URL: https://github.com/apache/spark/pull/37544#issuecomment-1217424108
cc @sunchao @maryannxue @beliefer
--
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
[GitHub] [spark] kazuyukitanimura commented on a diff in pull request #37544: [SPARK-40110][SQL][TESTS] Add JDBCWithAQESuite
Posted by GitBox <gi...@apache.org>.
kazuyukitanimura commented on code in PR #37544:
URL: https://github.com/apache/spark/pull/37544#discussion_r952186580
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -44,7 +45,8 @@ import org.apache.spark.sql.test.SharedSparkSession
import org.apache.spark.sql.types._
import org.apache.spark.util.Utils
-class JDBCSuite extends QueryTest with SharedSparkSession {
Review Comment:
In this PR, `JDBCSuite` runs without AQE enabled as now the suite inherits `DisableAdaptiveExecutionSuite`. In comparison, `JDBCWithAQESuite` inherits `EnableAdaptiveExecutionSuite`
--
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
[GitHub] [spark] kazuyukitanimura commented on a diff in pull request #37544: [SPARK-40110][SQL][TESTS] Add JDBCWithAQESuite
Posted by GitBox <gi...@apache.org>.
kazuyukitanimura commented on code in PR #37544:
URL: https://github.com/apache/spark/pull/37544#discussion_r953154117
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -44,7 +45,8 @@ import org.apache.spark.sql.test.SharedSparkSession
import org.apache.spark.sql.types._
import org.apache.spark.util.Utils
-class JDBCSuite extends QueryTest with SharedSparkSession {
Review Comment:
I see your point. I revisited this, and yes AQE is enabled. However, AQE was not actually not applied in the execution plan as `shouldApplyAQE()` returns `false`. In that sense, this PR description should have been `AQE applied` instead of `AQE enabled`.
--
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
[GitHub] [spark] beliefer commented on pull request #37544: [SPARK-40110][SQL][TESTS] Add JDBCWithAQESuite
Posted by GitBox <gi...@apache.org>.
beliefer commented on PR #37544:
URL: https://github.com/apache/spark/pull/37544#issuecomment-1218881222
@kazuyukitanimura Thank you for you ping.
--
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
[GitHub] [spark] kazuyukitanimura commented on a diff in pull request #37544: [SPARK-40110][SQL][TESTS] Add JDBCWithAQESuite
Posted by GitBox <gi...@apache.org>.
kazuyukitanimura commented on code in PR #37544:
URL: https://github.com/apache/spark/pull/37544#discussion_r954413572
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -44,7 +45,8 @@ import org.apache.spark.sql.test.SharedSparkSession
import org.apache.spark.sql.types._
import org.apache.spark.util.Utils
-class JDBCSuite extends QueryTest with SharedSparkSession {
Review Comment:
Got it. I missed that part. Let me send reverting PRs for those 3 test suites
--
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
[GitHub] [spark] sunchao closed pull request #37544: [SPARK-40110][SQL][TESTS] Add JDBCWithAQESuite
Posted by GitBox <gi...@apache.org>.
sunchao closed pull request #37544: [SPARK-40110][SQL][TESTS] Add JDBCWithAQESuite
URL: https://github.com/apache/spark/pull/37544
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #37544: [SPARK-40110][SQL][TESTS] Add JDBCWithAQESuite
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37544:
URL: https://github.com/apache/spark/pull/37544#discussion_r953362653
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -44,7 +45,8 @@ import org.apache.spark.sql.test.SharedSparkSession
import org.apache.spark.sql.types._
import org.apache.spark.util.Utils
-class JDBCSuite extends QueryTest with SharedSparkSession {
Review Comment:
`spark.sql.adaptive.forceApply` is designed for testing. Do you have use cases to turn it on in production? If use cases are valid, we should make it a public conf (instead of internal).
--
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
[GitHub] [spark] kazuyukitanimura commented on a diff in pull request #37544: [SPARK-40110][SQL][TESTS] Add JDBCWithAQESuite
Posted by GitBox <gi...@apache.org>.
kazuyukitanimura commented on code in PR #37544:
URL: https://github.com/apache/spark/pull/37544#discussion_r953154117
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -44,7 +45,8 @@ import org.apache.spark.sql.test.SharedSparkSession
import org.apache.spark.sql.types._
import org.apache.spark.util.Utils
-class JDBCSuite extends QueryTest with SharedSparkSession {
Review Comment:
I see your point. I revisited this, and yes AQE is enabled. However, AQE was not actually applied in the execution plan as `shouldApplyAQE()` returns `false`. In that sense, this PR description should have been `AQE applied` instead of `AQE enabled`.
--
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
[GitHub] [spark] kazuyukitanimura commented on a diff in pull request #37544: [SPARK-40110][SQL][TESTS] Add JDBCWithAQESuite
Posted by GitBox <gi...@apache.org>.
kazuyukitanimura commented on code in PR #37544:
URL: https://github.com/apache/spark/pull/37544#discussion_r953322660
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -44,7 +45,8 @@ import org.apache.spark.sql.test.SharedSparkSession
import org.apache.spark.sql.types._
import org.apache.spark.util.Utils
-class JDBCSuite extends QueryTest with SharedSparkSession {
Review Comment:
In that sense, I could have not to add `DisableAdaptiveExecutionSuite` in the base suite, so that it is closer to the 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
[GitHub] [spark] cloud-fan commented on a diff in pull request #37544: [SPARK-40110][SQL][TESTS] Add JDBCWithAQESuite
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37544:
URL: https://github.com/apache/spark/pull/37544#discussion_r952192971
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -44,7 +45,8 @@ import org.apache.spark.sql.test.SharedSparkSession
import org.apache.spark.sql.types._
import org.apache.spark.util.Utils
-class JDBCSuite extends QueryTest with SharedSparkSession {
Review Comment:
I mean before this PR, the test code didn't consider AQE but still passed. This is weird because AQE is on by 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
[GitHub] [spark] cloud-fan commented on a diff in pull request #37544: [SPARK-40110][SQL][TESTS] Add JDBCWithAQESuite
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37544:
URL: https://github.com/apache/spark/pull/37544#discussion_r952114808
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -44,7 +45,8 @@ import org.apache.spark.sql.test.SharedSparkSession
import org.apache.spark.sql.types._
import org.apache.spark.util.Utils
-class JDBCSuite extends QueryTest with SharedSparkSession {
Review Comment:
not related to this PR: AQE is on by default, how did this test suite still run with AQE off?
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -44,7 +45,8 @@ import org.apache.spark.sql.test.SharedSparkSession
import org.apache.spark.sql.types._
import org.apache.spark.util.Utils
-class JDBCSuite extends QueryTest with SharedSparkSession {
Review Comment:
not related to this PR: AQE is on by default, how did this test suite run with AQE off?
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #37544: [SPARK-40110][SQL][TESTS] Add JDBCWithAQESuite
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37544:
URL: https://github.com/apache/spark/pull/37544#discussion_r953252623
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -44,7 +45,8 @@ import org.apache.spark.sql.test.SharedSparkSession
import org.apache.spark.sql.types._
import org.apache.spark.util.Utils
-class JDBCSuite extends QueryTest with SharedSparkSession {
Review Comment:
does it mean this PR is testing something that will never happen in production?
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #37544: [SPARK-40110][SQL][TESTS] Add JDBCWithAQESuite
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37544:
URL: https://github.com/apache/spark/pull/37544#discussion_r954419078
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -44,7 +45,8 @@ import org.apache.spark.sql.test.SharedSparkSession
import org.apache.spark.sql.types._
import org.apache.spark.util.Utils
-class JDBCSuite extends QueryTest with SharedSparkSession {
Review Comment:
thanks for your understanding!
--
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
[GitHub] [spark] kazuyukitanimura commented on a diff in pull request #37544: [SPARK-40110][SQL][TESTS] Add JDBCWithAQESuite
Posted by GitBox <gi...@apache.org>.
kazuyukitanimura commented on code in PR #37544:
URL: https://github.com/apache/spark/pull/37544#discussion_r953317605
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -44,7 +45,8 @@ import org.apache.spark.sql.test.SharedSparkSession
import org.apache.spark.sql.types._
import org.apache.spark.util.Utils
-class JDBCSuite extends QueryTest with SharedSparkSession {
Review Comment:
It is actually opposite. This PR is adding the test for when user enables `spark.sql.adaptive.forceApply` that can happen in production.
--
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