[BUG] Aggregator jars are in the test classpath

[jlowe]

Originally the tests and integration tests were separated out from the sql-plugin project to ensure we were testing with the distribution jar and the shading that it was performing. That way we were testing with the same jar that users would use at runtime.

After the new v2 shims were introduced in 21.10, the aggregator jars now appear in the test and integration tests classpath which defeats the purpose of only having the distribution jar in the classpath. Effectively our test environment no longer closely reflects the environment that will be seen in real use cases.

The test code is referencing classes in the parallel-world setup which cannot be directly accessed via the dist jar, as those classes requires the application classloader which is unavailable at compile time. We need to find a way to compile the test code against these classes but run them solely against the distribution jar.

Fixing this will involve the completion of multiple tasks:

• Update CONTRIBUTING.md to describe where unit tests should be placed vs. integration tests
• Migrate unit tests under the top-level tests module to sql-plugin/src/tests
• Migrate integration-style tests under the top-level tests module to integration-tests/
• Remove the top-level tests/ module after all tests have been migrated to sql-plugin/tests or integration-tests/
• Update integration-tests jar dependencies to not require aggregator jars, relates to [BUG] Dependencies of published integration tests jar are missing #3934

[res-life]

@jlowe For this issue, the integration tests module should depend on dist.jar, but classes in dist.jar are in sub-directories like common3xx, spark301, spark3xx. I write a simple project to depend on dist artifact, actually can't import the classes. The dist is not a valid jar, the package and the path of a class should be the same, but there are extra root directories (common3xx, spark301, spark3xx...). After all, we use the idea IDE to develop. Even though we find a way to run the tests, the idea IDE can't recognize the classes in the dist jar.

Runtime is: java -classpath spark-home/jars/* : spark-plugin.jar
The jars in spark have high priority. It's better to use the same jars in the plugin as spark does.

The root cause we separated the tests from sql-plugin project originly is the jars conflict.
If we carefully tackle the dependencies, the integration python tests and the Scala tests will use some jars. And I think it's easy to use the jars as Spark does, just delete the dependencies that Spark shipped.

I suggest the following solution:
sql-plugin depends on spark-sql, remove the orc, flatbuffer, protobuffer, because spark-sql already shipped. Add shims for different conflicting jars.
Move the tests in the tests module to integration-tests or sql-plugin; Remove tests module.
integration-tests depends on sql-plugin, rapids-4-spark-shims, shuffle, udf jars.
Use the dist as a tool;
Use the aggregator as the dist.
aggregator depends on integration-tests, so we can guarantee that the aggregator will package the jars we have tested in the integration-tests.

[jlowe]

I write a simple project to depend on dist artifact, actually can't import the classes.

Accessing anything other than the very few, specifically public classes in the dist jar is not supported. Most classes are not public interfaces. A test that tries to access these classes is, by definition, not an integration test but more like a unit test.

After all, we use the idea IDE to develop. Even though we find a way to run the tests, the idea IDE can't recognize the classes in the dist jar.

If you use the dist jar to access the classes that we publicly support as part of the RAPIDS Accelerator API (e.g.: RapidsUDF) then you're fine. You're trying to access internal classes which is not supported. This is working as-designed and is not a bug.

integration-tests depends on sql-plugin, rapids-4-spark-shims, shuffle, udf jars.

Integration-tests must not depend on the sql-plugin project. If a test is trying to access internal classes in the dist jar, which is triggering the desire to depend on sql-plugin rather than the dist jar, then it is not an integration test since it is doing things a normal user could not do with the dist jar.

Use the dist as a tool;
Use the aggregator as the dist.

The aggregator project is used to build a jar artifact for the RAPIDS Accelerator built against a single, specific Spark version. The dist project can aggregate multiple aggregator jars into a single jar (using the parallel-world classpaths), resulting in a single jar that can run across Spark versions. Once we shim uses of ORC/Hive we can probably stop shading in the aggregator module, but I'm not sure we can use the aggregator module as the dist, nor would we want to since, again, we want to test against the artifact that we ship, not some intermediate artifact users cannot access. cc: @tgravescs @gerashegalov

[jlowe]

After discussing this offline with @res-life, the core problem is tests that need to access "inner" classes in sql-plugin and also create a RAPIDS Accelerated Spark session. We cannot create a RAPIDS Accelerated Spark session without the shims module, so these tests cannot go in the sql-plugin module. We cannot access inner classes from the dist jar, so these tests must have access to internal artifacts where the inner classes are still visible (like the aggregator jar). I'm not sure it's worth trying to rewrite these tests to avoid the inner class access or avoid the Spark session. If there's a straightforward way to compile these tests against the internal artifacts yet run against only the dist jar at runtime that may be worth it, but that seems to fly in the face of what Maven expects. Admittedly, It is weird to say you need to compile against artifact X but somehow don't need X to be in the classpath at runtime.

So ultimately this issue may be considered invalid given that the dist jar no longer allows direct access to inner classes.

[jlowe]

Closing this as wontfix. Due to the dist jar hiding almost all the sql-plugin classes, the tests require too much access to realistically work without the aggregator jars (or a lot of reflection use).