Copilot commented on code in PR #10376:
URL: https://github.com/apache/gravitino/pull/10376#discussion_r2917043210
##########
api/src/main/java/org/apache/gravitino/rel/indexes/Indexes.java:
##########
@@ -156,7 +157,7 @@ private IndexImpl(
this.indexType = indexType;
this.name = name;
this.fieldNames = fieldNames;
- this.properties = properties == null ? Map.of() : properties;
+ this.properties = properties == null ? Maps.newHashMap() : properties;
}
Review Comment:
`IndexImpl` assigns `properties == null ? Maps.newHashMap() : properties`
and then returns the internal map from `properties()`. Since `properties` is
part of `equals`/`hashCode`, mutating the returned map after construction can
violate those contracts. Recommend storing an immutable copy (e.g.,
`ImmutableMap.copyOf(properties)` and `ImmutableMap.of()` when null) or at
least wrapping with `Collections.unmodifiableMap(...)`.
##########
common/src/main/java/org/apache/gravitino/dto/rel/indexes/IndexDTO.java:
##########
@@ -57,7 +58,7 @@ public IndexDTO(
this.indexType = indexType;
this.name = name;
this.fieldNames = fieldNames;
- this.properties = properties == null ? Map.of() : properties;
+ this.properties = properties == null ? Maps.newHashMap() : properties;
}
Review Comment:
`IndexDTO` now defaults `properties` to `Maps.newHashMap()` when null. Since
the DTO’s `equals`/`hashCode` include `properties` and `properties()` exposes
the internal map, later mutation can break those contracts. Prefer using an
immutable empty map (e.g., `Collections.emptyMap()` / `ImmutableMap.of()`)
and/or defensively copy/wrap the provided map to keep the object stable.
##########
api/src/main/java/org/apache/gravitino/rel/indexes/Indexes.java:
##########
@@ -110,7 +111,7 @@ public static Index primary(String name, String[][]
fieldNames, Map<String, Stri
* @return An {@link Index} instance with empty properties
*/
public static Index of(IndexType indexType, String name, String[][]
fieldNames) {
- return of(indexType, name, fieldNames, Map.of());
+ return of(indexType, name, fieldNames, Maps.newHashMap());
}
Review Comment:
`Indexes.of(indexType, name, fieldNames)` now passes `Maps.newHashMap()` as
the default properties map. Because `properties()` is exposed directly and
participates in `equals`/`hashCode`, returning a mutable map can lead to
hard-to-debug issues if callers mutate it later (hashCode changes, broken
Set/Map lookups). Prefer an immutable empty map (e.g., `ImmutableMap.of()` /
`Collections.emptyMap()`), and consider defensively copying/wrapping the map
passed into `IndexImpl` to keep `Index` effectively immutable.
##########
common/src/main/java/org/apache/gravitino/dto/responses/DeleteResponse.java:
##########
@@ -26,7 +26,7 @@
* Represents a response for a delete operation. This class is deprecated and
will be removed in
* future versions, please use {@link DropResponse} instead.
*/
-@Deprecated(since = "1.0.0")
+@Deprecated()
Review Comment:
`@Deprecated()` is unusual style-wise; prefer `@Deprecated` without
parentheses for consistency and to avoid potential style/checkstyle violations.
```suggestion
@Deprecated
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
