HIVE-28888: Fix spotbugs issues in hive-storage-api and hive-service-rpc with spotbugs-maven-plugin 4.8.6.6 #5754

tanishq-chugh · 2025-04-08T18:38:56Z

What changes were proposed in this pull request?

Upgrade spotbugs-maven-plugin to 4.8.6.6

Why are the changes needed?

Upgraded plugin introduces new rules to identify potential bugs.

Does this PR introduce any user-facing change?

No

Is the change a dependency upgrade?

No, it is a plugin version upgrade.

How was this patch tested?

Locally, running spotbugs check and fixing the bugs, followed by running basic manual test on hive by building locally and running queries.

…y found spotbugs in the active modules

…en-plugin 4.8.6.6

okumin · 2025-04-16T10:09:51Z

storage-api/src/java/org/apache/hadoop/hive/common/ValidReadTxnList.java

+  public static ValidReadTxnList fromValue(String value) {
+    ValidReadTxnList txnList = new ValidReadTxnList();
+    txnList.readFromString(value);
+    return txnList;


I assume this is for CT_CONSTRUCTOR_THROW, as far as I tested it locally

Yes, this change is as well to address CT_CONSTRUCTOR_THROW spotbug.

okumin · 2025-04-16T10:11:00Z

Jenkinsfile

@@ -267,7 +264,7 @@ if [ $n != 0 ]; then
  exit 1
 fi
 '''
-        buildHive("-Pspotbugs -pl " + spotbugsProjects.join(",") + " -am test-compile com.github.spotbugs:spotbugs-maven-plugin:4.0.0:check")
+         buildHive("-Pspotbugs -pl " + spotbugsProjects.join(",") + " -am test-compile com.github.spotbugs:spotbugs-maven-plugin:4.8.6.6:check")


Suggested change

buildHive("-Pspotbugs -pl " + spotbugsProjects.join(",") + " -am test-compile com.github.spotbugs:spotbugs-maven-plugin:4.8.6.6:check")

buildHive("-Pspotbugs -pl " + spotbugsProjects.join(",") + " -am test-compile com.github.spotbugs:spotbugs-maven-plugin:4.8.6.6:check")

Thanks for pointing this out, made this change in be7e285

okumin · 2025-04-16T10:11:47Z

Jenkinsfile

+        ":hive-storage-api",
+        ":hive-service-rpc"


Suggested change

":hive-storage-api",

":hive-service-rpc"

":hive-storage-api",

":hive-service-rpc"

Thanks for pointing this out as well, made this change in be7e285

okumin · 2025-04-16T12:07:26Z

storage-api/src/java/org/apache/hive/common/util/BloomFilter.java

@@ -329,20 +331,19 @@ public static void mergeBloomFilterBytes(
   * for index bounds nor expand the bit set size if the specified index is greater than the size.
   */
  static class BitSet {
-    private final long[] data;


I guess we can keep this final. The new Spot bugs fail if the constructor might fail. If it never fails, it is ok.

// make it private private BitSet(long[] data) { this.data = data; // without assert here } public static BitSet build(long[] data) { assert data.length > 0 : "data length is zero!"; return new BitSet(data); }

Yes. This is definitely a better approach to solve this spotbug. Thanks for the suggestion. i have made the change in be7e285

okumin · 2025-04-16T12:14:18Z

storage-api/src/java/org/apache/hive/common/util/BloomKFilter.java

@@ -59,32 +60,44 @@ private static void checkArgument(boolean expression, String message) {
    }
  }

-  public BloomKFilter(long maxNumEntries) {
+  public BloomKFilter(int k, long m, int totalBlockCount, long[] arr) {


Suggested change

public BloomKFilter(int k, long m, int totalBlockCount, long[] arr) {

private BloomKFilter(int k, long m, int totalBlockCount, long[] arr) {

Made it private in be7e285

okumin · 2025-04-16T12:15:18Z

storage-api/src/java/org/apache/hive/common/util/BloomKFilter.java

@@ -397,21 +410,14 @@ public static byte[] getInitialBytes(long expectedEntries) {
   * for index bounds nor expand the bit set size if the specified index is greater than the size.
   */
  public static class BitSet {
-    private final long[] data;
+    private long[] data;


This can be final maybe

Yes. Made the field private as well as changed the constructor/build method similar to your suggested change for BloomFilter Bitset class. Change made in be7e285

okumin · 2025-04-16T12:34:28Z

Thanks. From my point of view, we'll have to do the following things when we close this pull request.

We have to include the JIRA ticket id in the PR title(= commit message)
We have to close the linked JIRA ticket with fix version = 4.1.0

I assume this PR includes HIVE-28888, HIVE-28889 and HIVE-28893. I expect the remaining tickets, meaning HIVE-28890, HIVE-28891, and HIVE-28892, not to be included.

I am thinking of consolidating tickets like below, keeping 1-1 mapping between PR and JIRA[1].

HIVE-28887(umbrella)
- HIVE-28888 -> Consolidate with HIVE-28889 and HIVE-28893
- HIVE-28889 -> VOID
- HIVE-28890 -> (Not yet done)
- HIVE-28891 -> (Not yet done)
- HIVE-28892 -> (Not yet done)
- HIVE-28893 -> VOID

Also, I added some minor comments. I ran spot bugs on my local machine with this change, and I've not seen any issues so far. Thanks!

[1] I've seen the case PR : JIRA = 1 : N but I don't know when we should accept it

tanishq-chugh · 2025-04-16T17:14:52Z

@okumin
Yes, that absolutely makes sense. I have changed the PR title to include the JIRA - HIVE-28888, with which we can go ahead as you mentioned. We'll make sure the upcoming PRs for this initiative have a 1:1 JIRA PR mapping.
Thank you!

okumin

@tanishq-chugh Thank you! Only one point. Could you please remove the unused import, import java.util.BitSet;? SonarCloud reported the issue.
I speculatively give this PR +1. I will merge it once the code style issue is resolved and also CI succeeds.

okumin · 2025-04-17T06:44:47Z

storage-api/src/java/org/apache/hive/common/util/BloomKFilter.java

@@ -26,6 +26,7 @@
 import java.io.OutputStream;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
+import java.util.BitSet;


Suggested change

import java.util.BitSet;

It looks like this is not used.
https://sonarcloud.io/project/issues?pullRequest=5754&open=AZYgu6tSMQfuFZWXHEBO&id=apache_hive

This has been addressed in 3ab7cae

okumin · 2025-04-17T06:59:56Z

storage-api/src/java/org/apache/hive/common/util/BloomKFilter.java

    this.totalBlockCount = bitSet.data.length / DEFAULT_BLOCK_SIZE;
  }
+  public static BloomKFilter build(long[] bits, int numFuncs) {
+    checkArgument((bits.length % DEFAULT_BLOCK_SIZE) == 0, "bitSet has to be block aligned");
+    return new BloomKFilter(bits,numFuncs);


Suggested change

return new BloomKFilter(bits,numFuncs);

return new BloomKFilter(bits, numFuncs);

Just align the style with other lines

This has been addressed in 3ab7cae

sonarqubecloud · 2025-04-17T19:59:07Z

Quality Gate passed

Issues
14 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

tanishq-chugh added 2 commits April 9, 2025 00:06

HIVE-28887: Upgrade spotbugs maven plugin to 4.8.6.6 and fix the newl…

3fe82b1

…y found spotbugs in the active modules

HIVE-28889: Fix spotbugs issues in hive-storage-api with spotbugs-mav…

6a1ce53

…en-plugin 4.8.6.6

asf-ci-hive added tests pending tests failed and removed tests pending labels Apr 8, 2025

tanishq-chugh force-pushed the spotbugs-upgrade-wip branch from 7f9a0c9 to 8471979 Compare April 8, 2025 19:13

asf-ci-hive added tests pending tests failed and removed tests failed tests pending labels Apr 8, 2025

ramitg254 force-pushed the spotbugs-upgrade-wip branch from 1dce396 to 156ef37 Compare April 9, 2025 13:28

asf-ci-hive added tests pending tests failed tests passed and removed tests failed tests pending labels Apr 9, 2025

HIVE-28893: Fix spotbugs issues in hive-service-api with spotbugs-mav…

4754a6c

…en-plugin 4.8.6.6

tanishq-chugh force-pushed the spotbugs-upgrade-wip branch from f88d053 to 4754a6c Compare April 11, 2025 07:04

asf-ci-hive removed the tests passed label Apr 11, 2025

okumin reviewed Apr 16, 2025

View reviewed changes

asf-ci-hive added tests pending and removed tests failed labels Apr 16, 2025

Address Review Comments

be7e285

asf-ci-hive added tests failed and removed tests pending labels Apr 16, 2025

tanishq-chugh changed the title ~~HIVE-28887: Fix spotbugs issues in hive-storage-api and hive-service-rpc with spotbugs-maven-plugin 4.8.6.6~~ HIVE-28888: Fix spotbugs issues in hive-storage-api and hive-service-rpc with spotbugs-maven-plugin 4.8.6.6 Apr 16, 2025

asf-ci-hive added tests pending tests unstable and removed tests failed tests pending tests unstable labels Apr 16, 2025

okumin approved these changes Apr 17, 2025

View reviewed changes

Address Review Comments - 2

3ab7cae

asf-ci-hive added tests failed tests pending tests unstable and removed tests pending tests failed tests unstable labels Apr 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIVE-28888: Fix spotbugs issues in hive-storage-api and hive-service-rpc with spotbugs-maven-plugin 4.8.6.6 #5754

HIVE-28888: Fix spotbugs issues in hive-storage-api and hive-service-rpc with spotbugs-maven-plugin 4.8.6.6 #5754

tanishq-chugh commented Apr 8, 2025 •

edited

Loading

okumin Apr 16, 2025

tanishq-chugh Apr 16, 2025

okumin Apr 16, 2025

tanishq-chugh Apr 16, 2025

okumin Apr 16, 2025

tanishq-chugh Apr 16, 2025

okumin Apr 16, 2025

tanishq-chugh Apr 16, 2025

okumin Apr 16, 2025

tanishq-chugh Apr 16, 2025

okumin Apr 16, 2025

tanishq-chugh Apr 16, 2025

okumin commented Apr 16, 2025

tanishq-chugh commented Apr 16, 2025

okumin left a comment

okumin Apr 17, 2025

tanishq-chugh Apr 17, 2025

okumin Apr 17, 2025

tanishq-chugh Apr 17, 2025

sonarqubecloud bot commented Apr 17, 2025

	buildHive("-Pspotbugs -pl " + spotbugsProjects.join(",") + " -am test-compile com.github.spotbugs:spotbugs-maven-plugin:4.8.6.6:check")
	buildHive("-Pspotbugs -pl " + spotbugsProjects.join(",") + " -am test-compile com.github.spotbugs:spotbugs-maven-plugin:4.8.6.6:check")

	public BloomKFilter(int k, long m, int totalBlockCount, long[] arr) {
	private BloomKFilter(int k, long m, int totalBlockCount, long[] arr) {

	return new BloomKFilter(bits,numFuncs);
	return new BloomKFilter(bits, numFuncs);

HIVE-28888: Fix spotbugs issues in hive-storage-api and hive-service-rpc with spotbugs-maven-plugin 4.8.6.6 #5754

Are you sure you want to change the base?

HIVE-28888: Fix spotbugs issues in hive-storage-api and hive-service-rpc with spotbugs-maven-plugin 4.8.6.6 #5754

Conversation

tanishq-chugh commented Apr 8, 2025 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

Is the change a dependency upgrade?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okumin commented Apr 16, 2025

tanishq-chugh commented Apr 16, 2025

okumin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 17, 2025

Quality Gate passed

tanishq-chugh commented Apr 8, 2025 •

edited

Loading