Copilot commented on code in PR #9927:
URL: https://github.com/apache/gravitino/pull/9927#discussion_r2787512386
##########
docs/jdbc-doris-catalog.md:
##########
@@ -171,7 +171,7 @@ Unsupported for now.
```java
Index[] indexes = new Index[] {
- Indexes.of(IndexType.PRIMARY_KEY, "PRIMARY", new String[][]{{"id"}})
+ Indexes.of(IndexType.PRIMARY_KEY, "PRIMARY", new String[][]{{"id"},
Map.of()})
}
Review Comment:
The Java example has a syntax error: `new String[][]{{"id"}, Map.of()}` puts
`Map.of()` inside the array initializer. `Map.of()` should be passed as the
separate `properties` argument to `Indexes.of(...)` after the `String[][]`.
##########
common/src/main/java/org/apache/gravitino/json/JsonUtils.java:
##########
@@ -1473,6 +1473,8 @@ public void serialize(Index value, JsonGenerator gen,
SerializerProvider seriali
}
gen.writeFieldName(INDEX_FIELD_NAMES);
gen.writeObject(value.fieldNames());
+ gen.writeFieldName("properties");
+ gen.writeObject(value.properties());
Review Comment:
`IndexSerializer` now always emits a `properties` field, but
`IndexDeserializer` still ignores it, so any non-empty properties will be
silently dropped on read/round-trip. Update `IndexDeserializer` to parse the
`properties` object (when present) and call `builder.withProperties(...)`,
defaulting to an empty map when absent for backward compatibility.
##########
api/src/main/java/org/apache/gravitino/rel/indexes/Index.java:
##########
@@ -44,6 +45,11 @@ public interface Index {
*/
String[][] fieldNames();
+ /**
+ * @return Extra properties for index configuration
+ */
+ Map<String, String> properties();
Review Comment:
Adding `properties()` as a new abstract method on the public `Index`
interface is source/binary incompatible for any external implementations of
`Index`. To keep the change backward-compatible (as stated in the PR
description), make this a `default` method returning an empty map (or introduce
a new sub-interface) so existing implementers continue to compile and run.
##########
docs/jdbc-oceanbase-catalog.md:
##########
@@ -190,7 +190,7 @@ Column[] cols = new Column[] {
Column.of("name", Types.VarCharType.of(500), "Name of the user", true,
false, null)
};
Index[] indexes = new Index[] {
- Indexes.of(IndexType.PRIMARY_KEY, "PRIMARY", new String[][]{{"id"}})
+ Indexes.of(IndexType.PRIMARY_KEY, "PRIMARY", new String[][]{{"id"}},
Map.of())
}
Review Comment:
In this Java snippet, the `Index[]` initializer is missing the trailing
semicolon (`};`). As written it won’t compile/copy-paste cleanly; please close
the array with `};` after the last element.
##########
docs/jdbc-oceanbase-catalog.md:
##########
@@ -227,8 +227,8 @@ Index[] indexes = new Index[] {
```java
Index[] indexes = new Index[] {
- Indexes.of(IndexType.PRIMARY_KEY, "PRIMARY", new String[][]{{"id"}}),
- Indexes.of(IndexType.UNIQUE_KEY, "id_name_uk", new String[][]{{"id"} ,
{"name"}}),
+ Indexes.of(IndexType.PRIMARY_KEY, "PRIMARY", new String[][]{{"id"}},
Map.of()),
+ Indexes.of(IndexType.UNIQUE_KEY, "id_name_uk", new String[][]{{"id"} ,
{"name"}}, Map.of()),
}
```
Review Comment:
This Java snippet’s `Index[]` initializer is missing the trailing semicolon
(`};`). Please end the array initializer with `};` so the example compiles when
copied.
##########
api/src/main/java/org/apache/gravitino/rel/indexes/Indexes.java:
##########
@@ -38,44 +40,62 @@ public class Indexes {
*
* @param name The name of the index
* @param fieldNames The field names under the table contained in the index.
+ * @param properties Extra properties for index configuration
* @return The unique index
*/
- public static Index unique(String name, String[][] fieldNames) {
- return of(Index.IndexType.UNIQUE_KEY, name, fieldNames);
+ public static Index unique(String name, String[][] fieldNames, Map<String,
String> properties) {
+ return of(Index.IndexType.UNIQUE_KEY, name, fieldNames, properties);
}
/**
* To create a MySQL primary key, you need to use the default primary key
name.
*
+ * @param properties Extra properties for index configuration
* @param fieldNames The field names under the table contained in the index.
* @return The primary key index
*/
- public static Index createMysqlPrimaryKey(String[][] fieldNames) {
- return primary(DEFAULT_PRIMARY_KEY_NAME, fieldNames);
+ public static Index createMysqlPrimaryKey(String[][] fieldNames, Map<String,
String> properties) {
+ return primary(DEFAULT_PRIMARY_KEY_NAME, fieldNames, properties);
}
/**
* Create a primary index on columns. Like primary (a), for complex like
primary
*
* @param name The name of the index
* @param fieldNames The field names under the table contained in the index.
+ * @param properties Extra properties for index configuration
* @return The primary index
*/
- public static Index primary(String name, String[][] fieldNames) {
- return of(Index.IndexType.PRIMARY_KEY, name, fieldNames);
+ public static Index primary(String name, String[][] fieldNames, Map<String,
String> properties) {
+ return of(Index.IndexType.PRIMARY_KEY, name, fieldNames, properties);
+ }
Review Comment:
`Indexes.unique(...)`, `Indexes.primary(...)`, and
`Indexes.createMysqlPrimaryKey(...)` were changed to require a `properties`
argument, which removes the previous overloads and breaks existing callers. To
preserve backward compatibility, keep the old signatures and add new overloads
that accept `properties`, delegating the old methods to the new ones with an
empty map.
##########
docs/jdbc-mysql-catalog.md:
##########
@@ -189,7 +189,7 @@ Column[] cols = new Column[] {
Column.of("name", Types.VarCharType.of(500), "Name of the user", true,
false, null)
};
Index[] indexes = new Index[] {
- Indexes.of(IndexType.PRIMARY_KEY, "PRIMARY", new String[][]{{"id"}})
+ Indexes.of(IndexType.PRIMARY_KEY, "PRIMARY", new String[][]{{"id"},
Map.of()})
};
Review Comment:
The Java example has a syntax error: `new String[][]{{"id"}, Map.of()}`
places `Map.of()` inside the 2D array initializer. It should pass `Map.of()` as
the fourth argument to `Indexes.of(...)` (after the `String[][]`), matching the
updated API signature.
##########
common/src/main/java/org/apache/gravitino/dto/rel/indexes/IndexDTO.java:
##########
@@ -167,6 +183,19 @@ public S withFieldNames(String[][] fieldNames) {
return (S) this;
}
+ /**
+ * Sets the properties of the index.
+ *
+ * @param properties The properties of the index.
+ * @return The builder.
+ */
+ public S withProperties(Map<String, String> properties) {
+ this.properties = properties;
+ return (S) this;
+ }
+
+ /** */
+
Review Comment:
There is an empty/placeholder JavaDoc block (`/** */`) left in the builder.
It doesn’t add information and should be removed to avoid clutter (and to keep
the file consistent with surrounding documentation).
```suggestion
```
##########
api/src/main/java/org/apache/gravitino/rel/indexes/Indexes.java:
##########
@@ -135,12 +168,13 @@ public boolean equals(Object o) {
IndexImpl index = (IndexImpl) o;
return indexType == index.indexType
&& Objects.equal(name, index.name)
- && Arrays.deepEquals(fieldNames, index.fieldNames);
+ && Arrays.deepEquals(fieldNames, index.fieldNames)
+ && Objects.equal(properties, index.properties);
}
@Override
public int hashCode() {
- return Objects.hashCode(indexType, name, Arrays.hashCode(fieldNames));
+ return Objects.hashCode(indexType, name, Arrays.hashCode(fieldNames),
properties);
Review Comment:
`IndexImpl.equals` uses `Arrays.deepEquals(fieldNames, ...)` but
`hashCode()` uses `Arrays.hashCode(fieldNames)`, which is not consistent for a
2D array and breaks the equals/hashCode contract (can cause incorrect behavior
in hash-based collections). Use a deep hash (e.g.,
`Arrays.deepHashCode(fieldNames)`) or manually combine per-row hashes to match
the equality logic.
```suggestion
return Objects.hashCode(indexType, name,
Arrays.deepHashCode(fieldNames), properties);
```
##########
common/src/test/java/org/apache/gravitino/json/TestSerializer.java:
##########
@@ -116,11 +117,13 @@ void testSortOrderSerializer() throws
JsonProcessingException {
void testIndexImplSerializer() throws JsonProcessingException {
IndexImpl index =
(IndexImpl)
- Indexes.of(IndexType.PRIMARY_KEY, "index_1", new String[][] {new
String[] {"col1"}});
+ Indexes.of(
+ IndexType.PRIMARY_KEY, "index_1", new String[][] {new String[]
{"col1"}}, Map.of());
String actualJson =
JsonUtils.anyFieldMapper().writeValueAsString((DTOConverters.toDTO(index)));
String expectedJson =
- "{\"indexType\":\"PRIMARY_KEY\",\"name\":\"index_1\"," +
"\"fieldNames\":[[\"col1\"]]}";
+ "{\"indexType\":\"PRIMARY_KEY\",\"name\":\"index_1\","
+ + "\"fieldNames\":[[\"col1\"]],\"properties\":{}}";
Assertions.assertEquals(expectedJson, actualJson);
Review Comment:
The updated serializer test only asserts that an empty `properties` map is
emitted. Since this PR’s main feature is supporting index properties, add at
least one test case with a non-empty properties map and assert it round-trips
through JSON (serialize + deserialize) to ensure properties aren’t dropped.
--
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]
