[SPARK-3873] [build] Add style checker to enforce import ordering.

[vanzin]

The checker tries to follow as closely as possible the guidelines of
the code style document, and makes some decisions where the guide is
not clear. In particular:

• wildcard imports come first when there are other imports in the
  same package
• multi-import blocks come before single imports
• lower-case names inside multi-import blocks come before others

In some projects, such as graphx, there seems to be a convention to
separate o.a.s imports from the project's own; to simplify the
checker, I chose not to allow that, which is a strict interpretation
of the code style guide, even though I think it makes sense.

Since the checks are based on syntax only, some edge cases may
generate spurious warnings; for example, when class names start
with a lower case letter (and are thus treated as a package name
by the checker).

The checker is currently only generating warnings, and since there
are many of those, the build output does get a little noisy. The
idea is to fix the code (and the checker, as needed) little by little
instead of having a huge change that touches everywhere.

[vanzin]

This is the same code I sent out a long time ago, updated to also hook up to the maven build. I tried both sbt and maven. There is a huge number of warnings, so I won't even try to fix them (in this change or a separate one), but since there seems to be a push to add more style checks, thought I might send this out again.

[SparkQA]

Test build #33752 has finished for PR 6502 at commit b3b87b1.

• This patch fails MiMa tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class ImportOrderChecker extends ScalariformChecker

[andrewor14]

Will this even pass tests in its current state? I imagine we violate the import ordering in a lot of places currently.

[rxin]

@andrewor14 he's doing warning level checks right now -- so there will be lots of warnings but still pass. Let's see how many warnings there are ...

[andrewor14]

I see... retest this please?

[SparkQA]

Test build #33764 has finished for PR 6502 at commit b3b87b1.

• This patch fails MiMa tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #33771 has finished for PR 6502 at commit b4dca33.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class ImportOrderChecker extends ScalariformChecker

[vanzin]

The PR builder script redirects the scalastyle output to a file and deletes it... so you don't see the warnings in the output and can't find them on the jenkins machines either.

But you'll see them if you try this change locally.

[rxin]

What happens to errors? Are they also redacted?

[vanzin]

See dev/scalastyle. The script greps for errors and prints them out, everything else is lost. It's trivial to change the script to keep the generated file with all errors and warnings.

[rxin]

@vanzin - thanks, this looks great.

Can you do 3 things?

1. Make the import ordering configurable.
2. The style rule is pretty complicated. Add some unit tests for it.
3. Submit a pull request against scalastyle proper.

The reason is we should avoid one-offs that make it harder to upgrade in the future (e.g. scalastyle sometimes change their internals, and then we'd need to learn how to upgrade this rule when we upgrade scalastyle). We can merge this as soon as there is a pull request against scalastyle and way to move forward. And once that is merged into scalastyle and scalastyle releases a new version, we can remove our one-off rule.

This is what we have done for all the one-off rules in the past.

[JoshRosen]

By the way, if you make this configurable then I'd model the configuration language / format after IntelliJ's import ordering configuration dialog.

[vanzin]

I filed a scalastyle PR (see link above) which contains tests, so I'll avoid adding those tests to this PR.

[SparkQA]

Test build #33932 has finished for PR 6502 at commit acff79c.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class ImportOrderChecker extends ScalariformChecker

[SparkQA]

Test build #35797 has finished for PR 6502 at commit 6f0bc17.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class ImportOrderChecker extends ScalariformChecker

[andrewor14]

LFTM. @rxin?

[JoshRosen]

@vanzin, it looks like the Scalastyle PR was merged: scalastyle/scalastyle#147

Given this, can you update this PR to configure Scalastyle to use this new rule?

[vanzin]

It was merged but I don't see a new scalastyle release. I was waiting for that before updating this pr.

[JoshRosen]

Can you comment on scalastyle/scalastyle#157 to voice your support for a new Scalastyle release? The delays in getting new Scalastyle and Scalastyle-SBT versions published are causing a lot of pain for folks running Scala 2.10.5+, since the current master also contains bugfixes for a Scalariform parser issue.

[JoshRosen]

Err, sorry, the link above should have been scalastyle/scalastyle#157

[JoshRosen]

Still no Scalastyle release :(

I tried bumping that ticket again.

[JoshRosen]

Now that Scalastyle 0.8.0 has been released, I think we can revive this PR. I've opened #10112 to bump the SBT and Scalastyle versions.

[vanzin]

I'm looking into it.

[vanzin]

I don't expect the tests to pass; there were style errors (unrelated to import ordering) when I tried things locally.

[SparkQA]

Test build #47108 has finished for PR 6502 at commit aded027.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):\n * class CrossValidator @Since(\"1.2.0\") (@Since(\"1.4.0\") override val uid: String)\n * class ParamGridBuilder @Since(\"1.2.0\")\n * class TrainValidationSplit @Since(\"1.5.0\") (@Since(\"1.5.0\") override val uid: String)\n

[SparkQA]

Test build #47149 has finished for PR 6502 at commit c1e5556.

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

[SparkQA]

Test build #47150 has finished for PR 6502 at commit 28e8287.

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

[SparkQA]

Test build #47274 has finished for PR 6502 at commit ce52797.

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

[vanzin]

not that it should matter, but retest this please

[SparkQA]

Test build #47355 has finished for PR 6502 at commit ce52797.

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

[vanzin]

I'm gonna merge this so we can start cleaning things up before enabling errors.