Checkstyle: prevent using unshaded Guava classes in Arrow and AssertJ

[jackye1995]

add new checkstyle rules for unshaded Guava classes for recently introduced libraries. @kbendick

[kbendick]

A few updates needed in the checkstyle descriptions and comments, but this is a good catch.

Is there anything in org.apache.arrow.util we might ever need?

I think it's safe to add this and then it can be reconsidered or made more explicit to the Preconditions check otherwise.

I know that my Intellij autoimports Preconditions from all sorts of places and it's easy to miss.

[kbendick]

Nit: The value here looks like it should maybe be BanUnrelocatedAssertjUtilClasses?

If we're looking to only ban the guava ones, maybe illegalClasses needs to be more specifc to org.apache.arrow.util.Preconditions?

EDIT: I checked and there's pretty much only Preconditions and other things available in our bundled-guava. Might need a unique id but otherwise this is fine.

[jackye1995]

I was mostly following this:

<module name="IllegalImport">
            <property name="id" value="BanUnrelocatedGuavaClasses"/>
            <property name="illegalPkgs" value="com.google.common"/>
            <message key="import.illegal" value="Use org.apache.iceberg.relocated.* classes from bundled-guava module instead."/>
        </module>

I am actually not sure if the ID has to be unique, let me check

[jackye1995]

Ah I see, so that's a user-defined name, then let me update it

[kbendick]

I checked myself when I was verifying this (not 100% used to updating these files), and they do not have to be unique.

It's just used in the output it seems, like this example where I added an import.

> Task :iceberg-arrow:checkstyleMain FAILED
[ant:checkstyle] [ERROR] /iceberg/arrow/src/main/java/org/apache/iceberg/arrow/ArrowAllocation.java:23:1:
Use org.apache.iceberg.relocated.* classes from bundled-guava module instead.
[BanUnrelocatedGuavaClasses]

[kbendick]

A few updates needed in the checkstyle descriptions and comments, but this is a good catch.

Is there anything in org.apache.arrow.util we might ever need?

I think it's safe to add this and then it can be reconsidered or made more explicit to the Preconditions check otherwise.

I know that my Intellij autoimports Preconditions from all sorts of places and it's easy to miss.

I went and checked the arrow library, and the only things I found in org.apache.arrow.util were Preconditions and something called AutoCloseables and something called Collections2 (a test class). Doubt we'll need those and we can update this if we do.

EDIT: I also went looking at org.assertj.core.util, and everything was copied from Guava or elsewhere. I think this is safe to do as well and we can also update it need be.

[kbendick]

cc @nastra who often updates the check style file, but I think this is safe and will help accidentally importing Preconditions and other Guava / google commons things from these libraries that also seem to have forked.

[kbendick]

I checked this out locally and run ./gradlew check and everything looked good. 👍

I also added an illegal import and checkstyle failed. Thanks @jackye1995.

[nastra]

changes make sense, what about preventing usage of org.apache.parquet.Preconditions? I believe this is also being used unintentionally at a few places

[jackye1995]

changes make sense, what about preventing usage of org.apache.parquet.Preconditions? I believe this is also being used unintentionally at a few places

sounds good, I also want to block the use of org.apache.commons.io.IOUtils in favor of Guava ByteStreams, let me put up another PR for these 2 to avoid too many changes in a single PR.

I will go ahead to merge this one, thanks for the reviews!