Skip to content

[SPARK-46237][SQL][TESTS] Make HiveDDLSuite independently testable #44153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
LuciferYang wants to merge 2 commits into from
Closed

[SPARK-46237][SQL][TESTS] Make HiveDDLSuite independently testable #44153

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd286ec
init
LuciferYang Dec 4, 2023
47e164e
address comments
LuciferYang Dec 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3270,7 +3270,10 @@ class HiveDDLSuite
val jarName = "TestUDTF.jar"
val jar = spark.asInstanceOf[TestHiveSparkSession].getHiveFile(jarName).toURI.toString
spark.sparkContext.allAddedJars.keys.find(_.contains(jarName))
.foreach(spark.sparkContext.addedJars("default").remove)
.foreach(k => spark.sparkContext.addedJars.get("default") match {

@LuciferYang LuciferYang Dec 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HyukjinKwon

I noticed that this logic was added in #41495. If I'm not mistaken, this is a defensive deletion to avoid including TestUDTF.jar in default during testing? However, I found that running only this test:

build/sbt "hive/testOnly org.apache.spark.sql.hive.execution.HiveDDLSuite" -Phive

throws an error:

[info] - SPARK-34261: Avoid side effect if create exists temporary function *** FAILED *** (4 milliseconds)
[info]   java.util.NoSuchElementException: key not found: default
[info]   at scala.collection.MapOps.default(Map.scala:274)
[info]   at scala.collection.MapOps.default$(Map.scala:273)
[info]   at scala.collection.AbstractMap.default(Map.scala:405)
[info]   at scala.collection.MapOps.apply(Map.scala:176)
[info]   at scala.collection.MapOps.apply$(Map.scala:175)
[info]   at scala.collection.AbstractMap.apply(Map.scala:405)
[info]   at org.apache.spark.sql.hive.execution.HiveDDLSuite.$anonfun$new$445(HiveDDLSuite.scala:3275)
[info]   at org.apache.spark.sql.test.SQLTestUtilsBase.withUserDefinedFunction(SQLTestUtils.scala:256)
[info]   at org.apache.spark.sql.test.SQLTestUtilsBase.withUserDefinedFunction$(SQLTestUtils.scala:254)
[info]   at org.apache.spark.sql.execution.command.DDLSuite.withUserDefinedFunction(DDLSuite.scala:326)
[info]   at org.apache.spark.sql.hive.execution.HiveDDLSuite.$anonfun$new$444(HiveDDLSuite.scala:3267)

I manually printed the contents of spark.sparkContext.addedJars and it's an empty Map.

But when I execute:

build/sbt "hive/testOnly org.apache.spark.sql.hive.execution.SQLQuerySuite org.apache.spark.sql.hive.execution.HiveDDLSuite" -Phive

all tests pass, and the content of spark.sparkContext.addedJars is:

Map(default -> Map(spark://localhost:54875/jars/SPARK-21101-1.0.jar -> 1701676986594, spark://localhost:54875/jars/hive-contrib-2.3.9.jar -> 1701676944590, spark://localhost:54875/jars/TestUDTF.jar -> 1701676921340))

In GitHub Action tests, SQLQuerySuite does indeed execute before HiveDDLSuite, so the failure is not reproduced.

So in this PR, I added a case match to only execute remove when default is not None.

I didn't directly clear all the contents in spark.sparkContext.allAddedJars after SQLQuerySuite in this PR, because I'm not sure about the impact of this behavior.

Do you think this is ok? Or do you have any better suggestions?

@HyukjinKwon HyukjinKwon Dec 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change seems fine. Can we do as below though?

foreach(k => spark.sparkContext.addedJars.get("default").foreach(_.remove(k)))

@LuciferYang LuciferYang Dec 4, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine to me ~

case Some(m) => m.remove(k)
case _ => // do nothing
})
assert(!spark.sparkContext.listJars().exists(_.contains(jarName)))
val e = intercept[AnalysisException] {
sql("CREATE TEMPORARY FUNCTION f1 AS " +
Expand Down