[#2164][#2166] improvement: enable MutablePublicArray and ReferenceEq…

…uality error-prone
apache · Mar 12, 2024 · 29c6b00 · 29c6b00
1 parent a9c6d2b
commit 29c6b00
Show file tree

Hide file tree

Showing 7 changed files with 18 additions and 6 deletions.
diff --git a/build.gradle.kts b/build.gradle.kts
@@ -272,6 +272,7 @@ subprojects {
         "MathRoundIntLong",
         "MissingSuperCall",
         "ModifyingCollectionWithItself",
+        "MutablePublicArray",
         "NonCanonicalStaticImport",
         "NonFinalCompileTimeConstant",
         "NonRuntimeAnnotation",
@@ -281,6 +282,7 @@ subprojects {
         "ParametersButNotParameterized",
         "RandomCast",
         "RandomModInteger",
+        "ReferenceEquality",
         "SelfAssignment",
         "SelfComparison",
         "SelfEquals",

diff --git a/...main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/ToIcebergType.java b/...main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/ToIcebergType.java
@@ -34,10 +34,12 @@ private int getNextId() {
     return nextId++;
   }
 
+  @SuppressWarnings("ReferenceEquality")
   @Override
   public Type struct(com.datastrato.gravitino.rel.types.Types.StructType struct, List<Type> types) {
     com.datastrato.gravitino.rel.types.Types.StructType.Field[] fields = struct.fields();
     List<Types.NestedField> newFields = Lists.newArrayListWithExpectedSize(fields.length);
+    // Comparing the root node by reference equality.
     boolean isRoot = root == struct;
     for (int i = 0; i < fields.length; i += 1) {
       com.datastrato.gravitino.rel.types.Types.StructType.Field field = fields[i];

diff --git a/common/src/test/java/com/datastrato/gravitino/config/TestConfigEntryList.java b/common/src/test/java/com/datastrato/gravitino/config/TestConfigEntryList.java
@@ -41,9 +41,7 @@ public void testConfWithDefaultValue() {
             .stringConf()
             .checkValue(value -> value == null, "error")
             .toSequence()
-            .checkValue(
-                valueList -> valueList.stream().allMatch(element -> element == "test-string"),
-                "error")
+            .checkValue(valueList -> valueList.stream().allMatch("test-string"::equals), "error")
             .createWithDefault(Lists.newArrayList("test-string", "test-string", "test-string"));
     List<String> valueList = testConf.readFrom(configMapEmpty);
     Assertions.assertEquals(null, configMapEmpty.get("gravitino.test.string.list"));

diff --git a/core/src/main/java/com/datastrato/gravitino/lock/LockManager.java b/core/src/main/java/com/datastrato/gravitino/lock/LockManager.java
@@ -218,6 +218,7 @@ void evictStaleNodes(TreeLockNode treeNode, TreeLockNode parent) {
    * @param identifier The identifier of the tree lock.
    * @return The created tree lock.
    */
+  @SuppressWarnings("ReferenceEquality")
   public TreeLock createTreeLock(NameIdentifier identifier) {
     checkTreeNodeIsFull();
 
@@ -227,6 +228,8 @@ public TreeLock createTreeLock(NameIdentifier identifier) {
       lockNode.addReference();
       treeLockNodes.add(lockNode);
 
+      // Avoid to use value equality instead of reference equality here.
+      // Otherwise, there will be an unexpected result when using NameIdentifier.of("/").
       if (identifier == ROOT) {
         // The lock tree root node
         return new TreeLock(treeLockNodes, identifier);

diff --git a/core/src/test/java/com/datastrato/gravitino/lock/TestLockManager.java b/core/src/test/java/com/datastrato/gravitino/lock/TestLockManager.java
@@ -649,4 +649,11 @@ void testDeadLockChecker() throws InterruptedException, ExecutionException {
       service.take().get();
     }
   }
+
+  @Test
+  public void testMockRootTreeLock() {
+    LockManager lockManager = new LockManager(getConfig());
+    lockManager.createTreeLock(NameIdentifier.of("/"));
+    Assertions.assertEquals(2L, lockManager.totalNodeCount.get(), "Should have 2 nodes");
+  }
 }
diff --git a/...connector/src/test/java/com/datastrato/gravitino/trino/connector/TestGravitinoConfig.java b/...connector/src/test/java/com/datastrato/gravitino/trino/connector/TestGravitinoConfig.java
@@ -42,7 +42,7 @@ public void testMissingConfig() {
       GravitinoConfig config = new GravitinoConfig(configMap);
       assertEquals(gravitinoUrl, config.getURI());
     } catch (TrinoException e) {
-      if (e.getErrorCode() != GRAVITINO_MISSING_CONFIG.toErrorCode()) {
+      if (GRAVITINO_MISSING_CONFIG.toErrorCode().equals(e.getErrorCode())) {
         throw e;
       }
     }

diff --git a/.../src/test/java/com/datastrato/gravitino/trino/connector/util/TestDataTypeTransformer.java b/.../src/test/java/com/datastrato/gravitino/trino/connector/util/TestDataTypeTransformer.java
@@ -95,7 +95,7 @@ public void testGetGravitinoType() {
     try {
       dataTypeTransformer.getGravitinoType(HyperLogLogType.HYPER_LOG_LOG);
     } catch (TrinoException e) {
-      if (e.getErrorCode() != GRAVITINO_UNSUPPORTED_TRINO_DATATYPE.toErrorCode()) {
+      if (!GRAVITINO_UNSUPPORTED_TRINO_DATATYPE.toErrorCode().equals(e.getErrorCode())) {
         throw e;
       }
     }
@@ -155,7 +155,7 @@ public void testGetTrinoType() {
     try {
       dataTypeTransformer.getTrinoType(Types.BinaryType.get());
     } catch (TrinoException e) {
-      if (e.getErrorCode() != GRAVITINO_UNSUPPORTED_GRAVITINO_DATATYPE.toErrorCode()) {
+      if (!GRAVITINO_UNSUPPORTED_GRAVITINO_DATATYPE.toErrorCode().equals(e.getErrorCode())) {
         throw e;
       }
     }