Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#2164][#2166] improvement: enable MutablePublicArray and ReferenceEquality error-prone #2511

Merged
merged 2 commits into from
Mar 12, 2024
Merged

[#2164][#2166] improvement: enable MutablePublicArray and ReferenceEquality error-prone #2511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29c6b00
[#2164][#2166] improvement: enable MutablePublicArray and ReferenceEq…
coolderli Mar 12, 2024
ec40734
update
coolderli Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ subprojects {
"MathRoundIntLong",
"MissingSuperCall",
"ModifyingCollectionWithItself",
"MutablePublicArray",
"NonCanonicalStaticImport",
"NonFinalCompileTimeConstant",
"NonRuntimeAnnotation",
Expand All @@ -281,6 +282,7 @@ subprojects {
"ParametersButNotParameterized",
"RandomCast",
"RandomModInteger",
"ReferenceEquality",
"SelfAssignment",
"SelfComparison",
"SelfEquals",
Expand Down
2 changes: 2 additions & 0 deletions ...main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/ToIcebergType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
4 changes: 1 addition & 3 deletions common/src/test/java/com/datastrato/gravitino/config/TestConfigEntryList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand Down
3 changes: 3 additions & 0 deletions core/src/main/java/com/datastrato/gravitino/lock/LockManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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);
Expand Down
7 changes: 7 additions & 0 deletions core/src/test/java/com/datastrato/gravitino/lock/TestLockManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,4 +649,11 @@ void testDeadLockChecker() throws InterruptedException, ExecutionException {
service.take().get();
}
}

@Test
public void testMockRootTreeLock() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to add this test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qqqttt123 I can remove it. I add it to validate https://github.com/datastrato/gravitino/pull/2511/files#diff-2144813a8300501a572d32e9f24651ab50594709644c031e983ac05a38bdc519R233.
if we replace == with .equals(), this unit test will failed. Do you think we need this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I got it.

LockManager lockManager = new LockManager(getConfig());
lockManager.createTreeLock(NameIdentifier.of("/"));
Assertions.assertEquals(2L, lockManager.totalNodeCount.get(), "Should have 2 nodes");
}
}
2 changes: 1 addition & 1 deletion ...connector/src/test/java/com/datastrato/gravitino/trino/connector/TestGravitinoConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down
4 changes: 2 additions & 2 deletions .../src/test/java/com/datastrato/gravitino/trino/connector/util/TestDataTypeTransformer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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;
}
}
Expand Down
Loading