[SPARK-33441][BUILD] Add unused-imports compilation check and remove all unused-imports #30351

LuciferYang · 2020-11-12T09:51:18Z

What changes were proposed in this pull request?

This pr add a new Scala compile arg to pom.xml to defense against new unused imports:

-Ywarn-unused-import for Scala 2.12
-Wconf:cat=unused-imports:e for Scala 2.13

The other fIles change are remove all unused imports in Spark code

Why are the changes needed?

Cleanup code and add guarantee to defense against new unused imports

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass the Jenkins or GitHub Action

HyukjinKwon · 2020-11-12T10:27:45Z

@LuciferYang, since you're here, can you see if -Ywarn-unused option works? We enabled now Xfatal-warnings by default so the warnings will be converted as errors.

SparkQA · 2020-11-12T11:00:21Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35594/

SparkQA · 2020-11-12T11:29:54Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35594/

LuciferYang · 2020-11-12T11:40:07Z

@HyukjinKwon Let me have a try ~

LuciferYang · 2020-11-12T12:17:56Z

@HyukjinKwon Looks like -Ywarn-unused can works, but there are some configuration differences between pom.xml and SparkBuild.scala, -Xfatal-warnings is only configured in SparkBuild.scala.

After add -Ywarn-unused to pom.xml, there are some waring messages as follows:

[WARNING] [Warn] /spark/core/src/main/scala-2.12/org/apache/spark/util/TimeStampedHashMap.scala:95: pattern var t in value $anonfun is never used; `t@_' suppresses this warning
[WARNING] [Warn] /spark/core/src/main/scala/org/apache/spark/BarrierTaskContext.scala:24: Unused import
[WARNING] [Warn] /spark/core/src/main/scala/org/apache/spark/ContextCleaner.scala:204: pattern var ie in method keepCleaning is never used; `ie@_' suppresses this warning
[WARNING] [Warn] /spark/core/src/main/scala/org/apache/spark/ContextCleaner.scala:252: parameter value blocking in method doCleanupAccum is never used
[WARNING] [Warn] /spark/core/src/main/scala/org/apache/spark/ExecutorAllocationClient.scala:100: parameter value triggeredByExecutor in method decommissionExecutors is never used
[WARNING] [Warn] /spark/core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala:326: private method totalRunningTasksPerResourceProfile in class ExecutorAllocationManager is never used
[WARNING] [Warn] /spark/core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala:784: pattern var k in value $anonfun is never used; `k@_' suppresses this warning
[WARNING] [Warn] /spark/core/src/main/scala/org/apache/spark/MapOutputTracker.scala:38: Unused import
[WARNING] [Warn] /spark/core/src/main/scala/org/apache/spark/MapOutputTracker.scala:139: pattern var e in method updateMapOutput is never used; `e@_' suppresses this warning
[WARNING] [Warn] /spark/core/src/main/scala/org/apache/spark/MapOutputTracker.scala:472: pattern var ie in method run is never used; `ie@_' suppresses this warning
[WARNING] [Warn] /spark/core/src/main/scala/org/apache/spark/MapOutputTracker.scala:698: pattern var loc in value $anonfun is never used; `loc@_' suppresses this warning
[WARNING] [Warn] /spark/core/src/main/scala/org/apache/spark/MapOutputTracker.scala:283: parameter value conf in class MapOutputTrackerMasterEndpoint is never used
[WARNING] [Warn] /spark/core/src/main/scala/org/apache/spark/MapOutputTracker.scala:308: parameter value conf in class MapOutputTracker is never used
[WARNING] [Warn] /spark/core/src/main/scala/org/apache/spark/MapOutputTracker.scala:426: parameter value i in value $anonfun is never used
[WARNING] [Warn] /spark/core/src/main/scala/org/apache/spark/MapOutputTracker.scala:519: parameter value x in value $anonfun is never used
[WARNING] [Warn] /spark/core/src/main/scala/org/apache/spark/MapOutputTracker.scala:671: parameter value numReducers in method getLocationsWithLargestOutputs is never used

After add -Ywarn-unused and -Xfatal-warnings to pom.xml , the[WARNING] convert to [ERROR], maybe we can use -Ywarn-unused-import to clean up unused imports first？

HyukjinKwon · 2020-11-12T12:27:38Z

Yeah, we can add it into pom.xml too. Yes, I think we can try with Ywarn-unused-import first. How many warnings/errors are there in the whole project?

SparkQA · 2020-11-12T12:37:41Z

Test build #130988 has finished for PR 30351 at commit dfeeb13.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

LuciferYang · 2020-11-12T14:10:55Z

Address 986ffe5 add unused-imports related compilation warning args, but there are some interesting things：

one is compiler think scala.language.xxx is unused import, such as import scala.language.postfixOps in BarrierTaskContext
on the other hand, compiler think imports used in comments is unused, such as import org.apache.spark.util.collection.OpenHashMap in ResourceAllocator

spark/core/src/main/scala/org/apache/spark/resource/ResourceAllocator.scala

Lines 23 to 44 in a3d2954

    
           import org.apache.spark.util.collection.OpenHashMap 
        
           /** 
        
            * Trait used to help executor/worker allocate resources. 
        
            * Please note that this is intended to be used in a single thread. 
        
            */ 
        
           trait ResourceAllocator { 
        
             protected def resourceName: String 
        
             protected def resourceAddresses: Seq[String] 
        
             protected def slotsPerAddress: Int 
        
             /** 
        
              * Map from an address to its availability, a value > 0 means the address is available, 
        
              * while value of 0 means the address is fully assigned. 
        
              * 
        
              * For task resources ([[org.apache.spark.scheduler.ExecutorResourceInfo]]), this value 
        
              * can be a multiple, such that each address can be allocated up to [[slotsPerAddress]] 
        
              * times. 
        
              * 
        
              * TODO Use [[OpenHashMap]] instead to gain better performance. 
        
              */

LuciferYang · 2020-11-12T14:11:55Z

Seems GitHub Action compile with -Xfatal-warnings default? all github action check failed .... haha, I will do statistics locally first @HyukjinKwon

SparkQA · 2020-11-12T15:03:38Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35610/

LuciferYang · 2020-11-12T15:14:58Z

@HyukjinKwon
compile with mvn clean install -DskipTests -Phadoop-3.2 -Phive-2.3 -Phadoop-cloud -Pmesos -Pyarn -Pkinesis-asl -Phive-thriftserver -Pspark-ganglia-lgpl -Pkubernetes -Phive

There are total 505 Unused import warnings printed with this pr, 291 in src dir and 214 in test dir, involves 308 files

HyukjinKwon · 2020-11-12T15:22:04Z

Yes, GitHub Actions build use SBT with Xfatal-warnings on by default. Can you file a JIRA and do it all in this PR?

HyukjinKwon · 2020-11-12T15:23:10Z

You'll probably have to specify -Xfatal-warnings in https://github.com/apache/spark/blob/master/project/SparkBuild.scala#L219

LuciferYang · 2020-11-12T15:31:49Z

Yes, GitHub Actions build use SBT with Xfatal-warnings on by default. Can you file a JIRA and do it all in this PR?

OK ~ It doesn't look like a minor anymore, It's a little late today in my timezone and I will try to finish the work tomorrow :)

SparkQA · 2020-11-12T15:34:46Z

Test build #131006 has finished for PR 30351 at commit 986ffe5.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-11-12T15:37:54Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35610/

SparkQA · 2020-11-13T08:05:02Z

Test build #131042 has finished for PR 30351 at commit 6fbf7c8.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

LuciferYang · 2020-11-18T13:42:58Z

@LuciferYang, let's leave it open for few more days in case other people have some more comments.

OK ~

SparkQA · 2020-11-18T14:51:37Z

Test build #131279 has finished for PR 30351 at commit f6f8cb7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

LuciferYang · 2020-11-18T16:54:39Z

Address d1d5f72 merge with master and Address 2addc8f fixed new added.

SparkQA · 2020-11-18T17:36:23Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35896/

SparkQA · 2020-11-18T18:05:50Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35896/

SparkQA · 2020-11-18T20:01:06Z

Test build #131294 has finished for PR 30351 at commit 2addc8f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-11-19T02:08:47Z

Okay, this gets conflicted easily. Let's merge this in, @LuciferYang can you resolve the conflicts?

SparkQA · 2020-11-19T02:51:27Z

Test build #131311 has finished for PR 30351 at commit 3498654.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ExecutorSource(
implicit class MetadataColumnsHelper(metadata: Array[MetadataColumn])

SparkQA · 2020-11-19T03:29:38Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35914/

LuciferYang · 2020-11-19T03:30:11Z

@HyukjinKwon done ~ Address 3498654 resolve the conflicts and Address ef2ff08 fix new added

SparkQA · 2020-11-19T04:00:02Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35914/

SparkQA · 2020-11-19T04:25:51Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35918/

SparkQA · 2020-11-19T04:50:04Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35918/

HyukjinKwon · 2020-11-19T05:20:29Z

Merged to master.

SparkQA · 2020-11-19T06:08:38Z

Test build #131315 has finished for PR 30351 at commit ef2ff08.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

LuciferYang · 2020-11-19T06:23:12Z

Thanks for your review ~ @HyukjinKwon @srowen

MaxGekk

Is dev/scalastyle supposed to catch unused imports?

$ ./dev/scalastyle
Scalastyle checks passed.

$ build/sbt -Phadoop-3.2 -Phive-2.3 -Pyarn -Phadoop-cloud -Phive-thriftserver -Pkubernetes -Pmesos -Phive -Pkinesis-asl -Pspark-ganglia-lgpl test:package streaming-kinesis-asl-assembly/assembly
[error] /Users/maximgekk/proj/show-partitions-exec-v2-test/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala:23:105: Unused import
[error] import org.apache.spark.sql.catalyst.analysis.{ResolvedNamespace, ResolvedPartitionSpec, ResolvedTable, ResolvedView}
[error]                                                                                                         ^
[error] /Users/maximgekk/proj/show-partitions-exec-v2-test/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ShowPartitionsExec.scala:20:29: Unused import
[error] import org.apache.spark.sql.AnalysisException
[error]                             ^
[error] two errors found
[error] (sql / Compile / compileIncremental) Compilation failed
[error] Total time: 32 s, completed Nov 19, 2020 6:30:02 PM

LuciferYang · 2020-11-19T15:43:40Z

@MaxGekk use org.scalastyle.scalariform.IllegalImportsChecker ? EDIT: seems not this rule

LuciferYang · 2020-11-19T15:57:43Z

@MaxGekk seems no corresponding rule scalastyle-rules

MaxGekk · 2020-11-19T16:15:17Z

Just in case, Maven doesn't detect unused imports too:

$ build/mvn -Phadoop-3.2 -Phive-2.3 -Pyarn -Phadoop-cloud -Phive-thriftserver -Pkubernetes -Pmesos -Phive -Pkinesis-asl -Pspark-ganglia-lgpl -DskipTests package
...
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  28:56 min
[INFO] Finished at: 2020-11-19T19:09:35+03:00
[INFO] ------------------------------------------------------------------------

LuciferYang · 2020-11-20T02:12:19Z

Yes, GitHub Actions build use SBT with -Xfatal-warnings on by default.

Just in case, Maven doesn't detect unused imports too:

@MaxGekk mvn compile maybe only have some compilation warnings about unused imports now, seems need add -Xfatal-warnings to maven pom.xml too, will this have other negative impact? @HyukjinKwon

LuciferYang · 2020-11-20T02:39:46Z

@HyukjinKwon @srowen @MaxGekk do you know how to add -P:silencer:globalFilters=.*deprecated.* to scala-maven-plugin ? There are still some differences in compilation options between maven and sbt:

maven:

<args>
    <arg>-unchecked</arg>
    <arg>-deprecation</arg>
    <arg>-feature</arg>
    <arg>-explaintypes</arg>
    <arg>-Ywarn-unused-import</arg>
    <arg>-target:jvm-1.8</arg>
 </args>

sbt:

Seq(
    "-Xfatal-warnings",
    "-deprecation",
    "-P:silencer:globalFilters=.*deprecated.*" //regex to catch deprecation warnings and supress them
)

seems need add -Xfatal-warnings and -P:silencer:globalFilters=.*deprecated.* to maven too, but will raise compile error if add scala-maven-plugin to <arg> directly as follows:

[ERROR] [Error] : bad option: -P:silencer:globalFilters=.*deprecated.*

And only add -Xfatal-warnings to maven will transform all warnings to errors

HyukjinKwon · 2020-11-20T02:46:07Z

Doing in SBT side only is fine. The purpose of doing this is to catch the unused imports in PRs.

HyukjinKwon · 2020-11-20T02:53:12Z

pom.xml

@@ -164,6 +164,7 @@
    <commons.collections.version>3.2.2</commons.collections.version>
    <scala.version>2.12.10</scala.version>
    <scala.binary.version>2.12</scala.binary.version>
+    <scalac.arg.unused-imports>-Ywarn-unused-import</scalac.arg.unused-imports>


Shall we just move it to SparkBuild.scala and let Maven doesn't care about that?

There should be only some warnings when maven build now, seems that @MaxGekk wants maven check it as error too?

Is my guess right? @MaxGekk

If it's easy to make both Maven and SBT build throw an error, it's fine. If that's difficult, let's move it to SparkBuild.scala, and make it SBT specific.

Shall we just move it to SparkBuild.scala and let Maven doesn't care about that?

It may be useful for Maven, it can also help Maven users to check this although it is only warnings with maven build

@HyukjinKwon SPARK-33560 added :)

Thanks @LuciferYang

@HyukjinKwon Is it necessary for us to add more compiler checking further? like unused-locals?

We can give a shot. Are they a lot of instances of unused-locals to fix? If there are too many, I think we should collect some more feedback from other committers because it makes more difficult to maintain the codes (e.g., reverting and backporting).

OK ~ I'll collect the details of the issue like unused-locals first, then file a new Jira for tracking and discussion. thanks @HyukjinKwon

remove unused imports in core module

dfeeb13

github-actions bot added CORE PYTHON R WEB UI labels Nov 12, 2020

add unused-imports compile arg

986ffe5

github-actions bot added the BUILD label Nov 12, 2020

remove unused imports core module

042c1dd

LuciferYang changed the title ~~[MINOR][CORE] Remove unused imports in core module~~ [SPARK-33441][CORE] Remove unused imports in core module Nov 13, 2020

LuciferYang added 2 commits November 13, 2020 15:49

remove unused imports sql module

3aa6fb4

remove -Xfatal-warnings to temp test

6fbf7c8

github-actions bot added SQL STRUCTURED STREAMING labels Nov 13, 2020

LuciferYang added 2 commits November 19, 2020 00:04

Merge branch 'upmaster' into remove-imports-core-module

d1d5f72

fix master new added

2addc8f

Merge branch 'upmaster' into remove-imports-core-module

3498654

fix new added

ef2ff08

HyukjinKwon closed this in e3058ba Nov 19, 2020

MaxGekk reviewed Nov 19, 2020

View reviewed changes

HyukjinKwon reviewed Nov 20, 2020

View reviewed changes

LuciferYang deleted the remove-imports-core-module branch June 6, 2022 03:45

[SPARK-33441][BUILD] Add unused-imports compilation check and remove all unused-imports #30351

[SPARK-33441][BUILD] Add unused-imports compilation check and remove all unused-imports #30351

Conversation

LuciferYang commented Nov 12, 2020 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

HyukjinKwon commented Nov 12, 2020

SparkQA commented Nov 12, 2020

SparkQA commented Nov 12, 2020

LuciferYang commented Nov 12, 2020

LuciferYang commented Nov 12, 2020 • edited Loading

HyukjinKwon commented Nov 12, 2020 • edited Loading

SparkQA commented Nov 12, 2020

LuciferYang commented Nov 12, 2020

LuciferYang commented Nov 12, 2020 • edited Loading

SparkQA commented Nov 12, 2020

LuciferYang commented Nov 12, 2020 • edited Loading

HyukjinKwon commented Nov 12, 2020

HyukjinKwon commented Nov 12, 2020

LuciferYang commented Nov 12, 2020

SparkQA commented Nov 12, 2020

SparkQA commented Nov 12, 2020

SparkQA commented Nov 13, 2020

LuciferYang commented Nov 18, 2020

SparkQA commented Nov 18, 2020

LuciferYang commented Nov 18, 2020

SparkQA commented Nov 18, 2020

SparkQA commented Nov 18, 2020

SparkQA commented Nov 18, 2020

HyukjinKwon commented Nov 19, 2020

SparkQA commented Nov 19, 2020

SparkQA commented Nov 19, 2020

LuciferYang commented Nov 19, 2020

SparkQA commented Nov 19, 2020

SparkQA commented Nov 19, 2020

SparkQA commented Nov 19, 2020

HyukjinKwon commented Nov 19, 2020

SparkQA commented Nov 19, 2020

LuciferYang commented Nov 19, 2020

MaxGekk left a comment

Choose a reason for hiding this comment

LuciferYang commented Nov 19, 2020 • edited Loading

LuciferYang commented Nov 19, 2020 • edited Loading

MaxGekk commented Nov 19, 2020

LuciferYang commented Nov 20, 2020

LuciferYang commented Nov 20, 2020 • edited Loading

HyukjinKwon commented Nov 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuciferYang commented Nov 12, 2020 •

edited

Loading

LuciferYang commented Nov 12, 2020 •

edited

Loading

HyukjinKwon commented Nov 12, 2020 •

edited

Loading

LuciferYang commented Nov 12, 2020 •

edited

Loading

LuciferYang commented Nov 12, 2020 •

edited

Loading

LuciferYang commented Nov 19, 2020 •

edited

Loading

LuciferYang commented Nov 19, 2020 •

edited

Loading

LuciferYang commented Nov 20, 2020 •

edited

Loading