Skip to content

[Minor][ML][MLLIB] Remove unused imports#12497

Closed
zhengruifeng wants to merge 13 commits intoapache:masterfrom
zhengruifeng:del_unused_imports
Closed

[Minor][ML][MLLIB] Remove unused imports#12497
zhengruifeng wants to merge 13 commits intoapache:masterfrom
zhengruifeng:del_unused_imports

Conversation

@zhengruifeng
Copy link
Contributor

@zhengruifeng zhengruifeng commented Apr 19, 2016

What changes were proposed in this pull request?

del unused imports in ML/MLLIB

How was this patch tested?

unit tests

@srowen
Copy link
Member

srowen commented Apr 19, 2016

Hm, is this the kind of thing we want to do in pretty big sweeps or not at all? There are tens of files that could have their imports trimmed.

@zhengruifeng
Copy link
Contributor Author

@srowen It seems most scala files dont have unused imports. If it's unnecessary, I will close this PR.

@zhengruifeng
Copy link
Contributor Author

cc @yanboliang

@SparkQA
Copy link

SparkQA commented Apr 19, 2016

Test build #56226 has finished for PR 12497 at commit 3750013.

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

@SparkQA
Copy link

SparkQA commented Apr 19, 2016

Test build #56232 has finished for PR 12497 at commit de34bae.

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

@SparkQA
Copy link

SparkQA commented Apr 19, 2016

Test build #56233 has finished for PR 12497 at commit 2ca5efd.

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

@rxin
Copy link
Contributor

rxin commented Apr 19, 2016

Yea - I think we either try to do this at scale or not do it at all. Thanks for the contribution.

@zhengruifeng
Copy link
Contributor Author

zhengruifeng commented Apr 19, 2016

I have manually reviewed all scala imports in ML/MLLib twice. IMO that is all.

@zhengruifeng zhengruifeng changed the title [Minor][ML] Remove unused imports [Minor][ML][MLLIB] Remove unused imports Apr 19, 2016
@SparkQA
Copy link

SparkQA commented Apr 19, 2016

Test build #56294 has finished for PR 12497 at commit b226bc6.

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

@SparkQA
Copy link

SparkQA commented Apr 20, 2016

Test build #56295 has finished for PR 12497 at commit 8aff210.

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

@srowen
Copy link
Member

srowen commented Apr 20, 2016

Since this is something that would be good to do globally, if at all, and there are automated tools to find this type of problem, I'd run an IDE over this to find and fix them in one go. For example, I pushed a button in IDEA and found more unused imports in:

ExternalShuffleBlockHandlerSuite.java
SparkSubmitOptionParserSuite.java
TransportResponseHandlerSuite.java
UnsafeRow.java

Although it also easily flags unused imports in Scala in the editor, I couldn't find an inspection for it. It does let you automatically optimize imports in Scala, though by default it will reorder them and other things, which can be disabled. It still seems like a hard way to just discover unused imports without changing a lot of stuff. For now if we can get as many fixed in one go as is reasonably possible, maybe that's sufficient.

@zhengruifeng
Copy link
Contributor Author

@srowen IDEA's unused hint may be wrong in some place.
In ParamGridBuilder.scala, IDEA flags 'scala.annotation.varargs' as unused, which is actually needed in build.

@srowen
Copy link
Member

srowen commented Apr 20, 2016

Agree, of course we have to test the result of removing "unused" imports in any event, and that's less obvious for Scala. It gets it right most of the time. In any event, the Java inspections are reliable.

@srowen
Copy link
Member

srowen commented Apr 22, 2016

@zhengruifeng this one needs a rebase, and you can remove more unused imports per above while you're at it

@zhengruifeng
Copy link
Contributor Author

@srowen I have reviewed all scala files in Graphx and some in SQL. And remove another some unused imports in this PR.

@SparkQA
Copy link

SparkQA commented Apr 23, 2016

Test build #56778 has finished for PR 12497 at commit ab42268.

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

@rxin
Copy link
Contributor

rxin commented Apr 23, 2016

Thanks - merging in master.

@asfgit asfgit closed this in 86ca8fe Apr 23, 2016
@SparkQA
Copy link

SparkQA commented Apr 23, 2016

Test build #56777 has finished for PR 12497 at commit 90e57c8.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@zhengruifeng zhengruifeng deleted the del_unused_imports branch April 23, 2016 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants