[SPARK-36362][CORE][SQL][TESTS] Omnibus Java code static analyzer warning fixes #33594

srowen · 2021-07-30T17:26:23Z

What changes were proposed in this pull request?

Fix up some minor Java issues:

Some int*int multiplications that widen to long maybe could overflow
Unnecessarily non-static inner classes
Some tests "catch (AssertionError)" and do nothing
Manual array iteration vs very slightly faster/simpler foreach
Incorrect generic types that just happen to not cause a runtime error
Missed opportunities for try-close
Mutable enums
.. and a few other minor things

Why are the changes needed?

Some are minor but clear fixes; some may have a marginal perf impact or avoid a bug later. Also: maybe avoid future PRs to address these one by one.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests

- Unnecessarily non-static inner classes - Some tests "catch (AssertionError)" and do nothing - Manual array iteration vs very slightly faster/simpler foreach - Incorrect generic types that just happen to not cause a runtime error - Missed opportunities for try-close - Mutable enums - .. and a few other minor things

srowen · 2021-07-30T17:27:06Z

common/kvstore/src/main/java/org/apache/spark/util/kvstore/KVTypeInfo.java

@@ -130,7 +130,7 @@ String getParentIndexName(String indexName) {
    Class<?> getType();
  }

-  private class FieldAccessor implements Accessor {
+  private static class FieldAccessor implements Accessor {


static is better for inner classes that need no reference to the outer class. While I don't think it comes up -- those hidden references can cause GC or serialization issues in theory. Just cleaner.

srowen · 2021-07-30T17:29:05Z

...n/network-common/src/test/java/org/apache/spark/network/protocol/MessageWithHeaderSuite.java

@@ -150,7 +150,7 @@ private ByteBuf doWrite(MessageWithHeader msg, int minExpectedWrites) throws Exc

    @Override
    public long count() {
-      return 8 * writeCount;
+      return 8L * writeCount;


The idea here is that 8 * writeCount is int * int, which could overflow, even though the result is meant to be long. Cast before the multiply. Probably not actually a bug in practice but who knows.

srowen · 2021-07-30T17:32:12Z

core/src/test/java/org/apache/spark/unsafe/map/AbstractBytesToBytesMapSuite.java

@@ -217,19 +217,6 @@ public void setAndRetrieveAKey() {
      Assert.assertArrayEquals(valueData,
        getByteArray(loc.getValueBase(), loc.getValueOffset(), recordLengthBytes));

-      try {


This test has the same problem as the above, but, is actually further just 'wrong' since a long time ago, when putNewValue was replaced by append. It is no longer expected to assert, or that's what I work out from the commit log.

srowen · 2021-07-30T17:32:51Z

...lyst/src/test/java/org/apache/spark/sql/catalyst/expressions/RowBasedKeyValueBatchSuite.java

      try {
        batch.getKeyRow(-1);
-        Assert.fail("Should not be able to get row -1");


Same problem -- if it 'failed' it would still pass as the fail() is caught by the catch block

srowen · 2021-07-30T17:33:02Z

sql/core/src/test/java/test/org/apache/spark/sql/JavaDatasetSuite.java

@@ -1548,7 +1548,7 @@ public void test() {
    A("www.elgoog.com"),
    B("www.google.com");

-    private String url;
+    private final String url;


Enums shouldn't be mutable!

srowen · 2021-07-30T17:33:20Z

streaming/src/test/java/test/org/apache/spark/streaming/Java8APISuite.java

@@ -547,7 +547,7 @@ public void testPairMap2() { // Maps pair -> single
    JavaPairDStream<String, Integer> pairStream = JavaPairDStream.fromJavaDStream(stream);
    JavaDStream<Integer> reversed = pairStream.map(Tuple2::_2);
    JavaTestUtils.attachTestOutputStream(reversed);
-    List<List<Tuple2<Integer, String>>> result = JavaTestUtils.runStreams(ssc, 2, 2);
+    List<List<Integer>> result = JavaTestUtils.runStreams(ssc, 2, 2);


The generic types were just wrong. It happens to work because of erasure.

SparkQA · 2021-07-30T18:21:09Z

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

SparkQA · 2021-07-30T18:54:30Z

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

SparkQA · 2021-07-30T19:59:40Z

Test build #141907 has finished for PR 33594 at commit b4b0f4a.

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

dongjoon-hyun

+1, LGTM. Thank you, @srowen .
It seems that the pattern of AssertionError are easily slipped during review.

dongjoon-hyun · 2021-08-01T05:36:47Z

Merged to master for Apache Spark 3.3.0 only.

dongjoon-hyun · 2021-08-01T07:42:05Z

Oh, there are linter errors.

src/test/java/org/apache/spark/util/kvstore/DBIteratorSuite.java:[76] (sizes) LineLength: Line is longer than 100 characters (found 104).
Error:  src/test/java/org/apache/spark/network/crypto/AuthEngineSuite.java:[22,8] (imports) UnusedImports: Unused import - java.util.Arrays.
Error:  src/test/java/org/apache/spark/network/RpcIntegrationSuite.java:[308] (sizes) LineLength: Line is longer than 100 characters (found 107).
Error:  src/test/java/org/apache/spark/network/util/NettyMemoryMetricsSuite.java:[111] (sizes) LineLength: Line is longer than 100 characters (found 107).
Error:  src/test/java/org/apache/spark/network/StreamSuite.java:[27,8] (imports) UnusedImports: Unused import - java.util.Arrays.
Error:  src/test/java/org/apache/spark/network/StreamSuite.java:[112] (sizes) LineLength: Line is longer than 100 characters (found 107).
Error:  src/test/java/org/apache/spark/network/StreamSuite.java:[121] (sizes) LineLength: Line is longer than 100 characters (found 107).
Error:  src/test/java/org/apache/spark/network/StreamSuite.java:[130] (sizes) LineLength: Line is longer than 100 characters (found 107).
Error:  src/test/java/org/apache/spark/network/StreamSuite.java:[144] (sizes) LineLength: Line is longer than 100 characters (found 107).
Error:  src/main/java/org/apache/spark/unsafe/types/UTF8String.java:[909] (sizes) LineLength: Line is longer than 100 characters (found 104).
Error:  src/main/java/org/apache/spark/unsafe/types/UTF8String.java:[942] (sizes) LineLength: Line is longer than 100 characters (found 104).
Error:  src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java:[403] (sizes) LineLength: Line is longer than 100 characters (found 101).
Error:  src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java:[404] (sizes) LineLength: Line is longer than 100 characters (found 102).
Error:  src/test/java/test/org/apache/spark/sql/JavaColumnExpressionSuite.java:[63] (sizes) LineLength: Line is longer than 100 characters (found 102).
Error:  src/test/java/test/org/apache/spark/sql/JavaColumnExpressionSuite.java:[66] (sizes) LineLength: Line is longer than 100 characters (found 102).
Error:  src/test/java/test/org/apache/spark/sql/JavaColumnExpressionSuite.java:[68] (sizes) LineLength: Line is longer than 100 characters (found 102).
Error:  src/test/java/test/org/apache/spark/sql/JavaColumnExpressionSuite.java:[69] (sizes) LineLength: Line is longer than 100 characters (found 102).

dongjoon-hyun · 2021-08-01T07:42:30Z

I'll make a follow-up.

dongjoon-hyun · 2021-08-01T07:50:37Z

Here is the PR.

[SPARK-36362][CORE][SQL][FOLLOWUP] Fix java linter errors #33601

### What changes were proposed in this pull request? This is a follow-up of #33594 to fix the Java linter error. ### Why are the changes needed? To recover GitHub Action. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the GitHub Action. Closes #33601 from dongjoon-hyun/SPARK-36362. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

…ning fixes Fix up some minor Java issues: - Some int*int multiplications that widen to long maybe could overflow - Unnecessarily non-static inner classes - Some tests "catch (AssertionError)" and do nothing - Manual array iteration vs very slightly faster/simpler foreach - Incorrect generic types that just happen to not cause a runtime error - Missed opportunities for try-close - Mutable enums - .. and a few other minor things Some are minor but clear fixes; some may have a marginal perf impact or avoid a bug later. Also: maybe avoid future PRs to address these one by one. No. Existing tests Closes #33594 from srowen/SPARK-36362. Authored-by: Sean Owen <srowen@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

### What changes were proposed in this pull request? This is a follow-up of #33594 to fix the Java linter error. ### Why are the changes needed? To recover GitHub Action. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the GitHub Action. Closes #33601 from dongjoon-hyun/SPARK-36362. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

github-actions bot added CORE DSTREAM SQL labels Jul 30, 2021

srowen added 2 commits July 30, 2021 12:28

Restore one close() in test

5944d26

Fix indent

b4b0f4a

srowen commented Jul 30, 2021

View reviewed changes

dongjoon-hyun approved these changes Aug 1, 2021

View reviewed changes

dongjoon-hyun closed this in 72615bc Aug 1, 2021

dongjoon-hyun mentioned this pull request Aug 1, 2021

[SPARK-36362][CORE][SQL][FOLLOWUP] Fix java linter errors #33601

Closed

srowen deleted the SPARK-36362 branch September 10, 2021 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-36362][CORE][SQL][TESTS] Omnibus Java code static analyzer warning fixes #33594

[SPARK-36362][CORE][SQL][TESTS] Omnibus Java code static analyzer warning fixes #33594

srowen commented Jul 30, 2021

srowen Jul 30, 2021

srowen Jul 30, 2021

srowen Jul 30, 2021

srowen Jul 30, 2021

srowen Jul 30, 2021

srowen Jul 30, 2021

SparkQA commented Jul 30, 2021

SparkQA commented Jul 30, 2021

SparkQA commented Jul 30, 2021

dongjoon-hyun left a comment

dongjoon-hyun commented Aug 1, 2021

dongjoon-hyun commented Aug 1, 2021

dongjoon-hyun commented Aug 1, 2021

dongjoon-hyun commented Aug 1, 2021

[SPARK-36362][CORE][SQL][TESTS] Omnibus Java code static analyzer warning fixes #33594

[SPARK-36362][CORE][SQL][TESTS] Omnibus Java code static analyzer warning fixes #33594

Conversation

srowen commented Jul 30, 2021

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?

srowen Jul 30, 2021

Choose a reason for hiding this comment

srowen Jul 30, 2021

Choose a reason for hiding this comment

srowen Jul 30, 2021

Choose a reason for hiding this comment

srowen Jul 30, 2021

Choose a reason for hiding this comment

srowen Jul 30, 2021

Choose a reason for hiding this comment

srowen Jul 30, 2021

Choose a reason for hiding this comment

SparkQA commented Jul 30, 2021

SparkQA commented Jul 30, 2021

SparkQA commented Jul 30, 2021

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Aug 1, 2021

dongjoon-hyun commented Aug 1, 2021

dongjoon-hyun commented Aug 1, 2021

dongjoon-hyun commented Aug 1, 2021