This is an automated email from the ASF dual-hosted git repository.
jerryshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 14e99c3842 [#13230] fix(api): defensively copy fieldNames and
properties in IndexImpl (#13231)
14e99c3842 is described below
commit 14e99c3842236afbc234abfeec62cf02646131c0
Author: YangJie <[email protected]>
AuthorDate: Mon Sep 21 23:30:22 2026 -0400
[#13230] fix(api): defensively copy fieldNames and properties in IndexImpl
(#13231)
### What changes were proposed in this pull request?
`IndexImpl` now deep-copies `fieldNames` at construction and on
`fieldNames()`, preserving null rows (`row == null ? null :
row.clone()`), and stores `properties` as a null-tolerant unmodifiable
copy (`Collections.unmodifiableMap(new HashMap<>(properties))`, matching
the sibling `TableChange.AddIndex`). `equals`/`hashCode` semantics are
unchanged.
### Why are the changes needed?
The index stored and returned the caller's `String[][]` and `Map` by
reference, so mutating the caller's arrays before or after `Indexes.of`
changed the built index and drifted its equality. The defensive copy
keeps a built index immutable while still accepting the inputs the old
code accepted, including a null field-name row and null property values
(which `Arrays.deepEquals`/`deepHashCode` and the map comparison
handle).
Fix: #13230
### Does this PR introduce _any_ user-facing change?
No API change. A built index is now immutable: mutating the caller's
arrays or map no longer changes the index. Inputs that were previously
accepted (including a null field-name row or null property value) are
still accepted. `equals`/`hashCode` semantics are unchanged.
### How was this patch tested?
Added `TestIndexes` cases that pin: mutating the caller-supplied arrays
and map does not change the built index; a null field-name row is
accepted and compares correctly; and a null property value is accepted.
They fail on the pre-fix tree and pass after the fix.
---------
Co-authored-by: Jerry Shao <[email protected]>
---
.../org/apache/gravitino/rel/indexes/Indexes.java | 25 +++++-
.../apache/gravitino/rel/indexes/TestIndexes.java | 100 +++++++++++++++++++++
.../lakehouse/paimon/TestGravitinoPaimonTable.java | 2 +-
3 files changed, 122 insertions(+), 5 deletions(-)
diff --git a/api/src/main/java/org/apache/gravitino/rel/indexes/Indexes.java
b/api/src/main/java/org/apache/gravitino/rel/indexes/Indexes.java
index 79fe95ef01..630c7ed73c 100644
--- a/api/src/main/java/org/apache/gravitino/rel/indexes/Indexes.java
+++ b/api/src/main/java/org/apache/gravitino/rel/indexes/Indexes.java
@@ -21,6 +21,8 @@ package org.apache.gravitino.rel.indexes;
import com.google.common.base.Objects;
import com.google.common.collect.ImmutableMap;
import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
import java.util.Map;
import org.apache.gravitino.rel.indexes.Index.IndexType;
@@ -155,8 +157,19 @@ public class Indexes {
IndexType indexType, String name, String[][] fieldNames, Map<String,
String> properties) {
this.indexType = indexType;
this.name = name;
- this.fieldNames = fieldNames;
- this.properties = properties == null ? ImmutableMap.of() : properties;
+ // Deep-copy the caller's arrays and snapshot the map so later external
mutations of the
+ // caller's inputs cannot change the built index. Null rows and null map
entries are
+ // preserved rather than rejected, matching the pre-fix behavior and
TableChange.AddIndex.
+ this.fieldNames =
+ fieldNames == null
+ ? null
+ : Arrays.stream(fieldNames)
+ .map(row -> row == null ? null : row.clone())
+ .toArray(String[][]::new);
+ this.properties =
+ properties == null
+ ? ImmutableMap.of()
+ : Collections.unmodifiableMap(new HashMap<>(properties));
}
/**
@@ -176,11 +189,15 @@ public class Indexes {
}
/**
- * @return The field names under the table contained in the index
+ * @return A defensive copy of the field names under the table contained
in the index
*/
@Override
public String[][] fieldNames() {
- return fieldNames;
+ return fieldNames == null
+ ? null
+ : Arrays.stream(fieldNames)
+ .map(row -> row == null ? null : row.clone())
+ .toArray(String[][]::new);
}
/**
diff --git
a/api/src/test/java/org/apache/gravitino/rel/indexes/TestIndexes.java
b/api/src/test/java/org/apache/gravitino/rel/indexes/TestIndexes.java
new file mode 100644
index 0000000000..428449b878
--- /dev/null
+++ b/api/src/test/java/org/apache/gravitino/rel/indexes/TestIndexes.java
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.rel.indexes;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.HashMap;
+import java.util.Map;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestIndexes {
+
+ @Test
+ public void testIndexDefensiveCopies() {
+ String[][] fieldNames = {{"col1"}};
+ Map<String, String> properties = new HashMap<>();
+ properties.put("key", "value");
+
+ Index index = Indexes.of(Index.IndexType.PRIMARY_KEY, "idx", fieldNames,
properties);
+
+ // Before the fix, IndexImpl stored the caller's arrays and map by
reference and returned them
+ // directly, so external mutation leaked into the built index.
+ fieldNames[0][0] = "mutated";
+ properties.put("other", "injected");
+
+ Assertions.assertArrayEquals(new String[] {"col1"}, index.fieldNames()[0]);
+ Assertions.assertEquals(ImmutableMap.of("key", "value"),
index.properties());
+ }
+
+ @Test
+ public void testIndexReturnedCollectionsAreIsolated() {
+ Index index =
+ Indexes.of(
+ Index.IndexType.UNIQUE_KEY,
+ "idx",
+ new String[][] {{"a", "b"}},
+ ImmutableMap.of("k", "v"));
+
+ // Mutating what the accessor returned must not change the index state
either.
+ index.fieldNames()[0][0] = "mutated";
+ Assertions.assertEquals("a", index.fieldNames()[0][0]);
+ Assertions.assertThrows(
+ UnsupportedOperationException.class, () ->
index.properties().put("k2", "v2"));
+ }
+
+ @Test
+ public void testNullFieldNameRowIsTolerated() {
+ // A null row must not blow up the defensive copy in either the
constructor or fieldNames().
+ String[][] fieldNames = {{"col1"}, null};
+
+ Index index = Indexes.of(Index.IndexType.PRIMARY_KEY, "idx", fieldNames,
ImmutableMap.of());
+
+ String[][] returned = index.fieldNames();
+ Assertions.assertArrayEquals(new String[] {"col1"}, returned[0]);
+ Assertions.assertNull(returned[1]);
+
+ // equals/hashCode must stay null-safe across the null row.
+ Index same =
+ Indexes.of(
+ Index.IndexType.PRIMARY_KEY, "idx", new String[][] {{"col1"},
null}, ImmutableMap.of());
+ Assertions.assertEquals(index, same);
+ Assertions.assertEquals(index.hashCode(), same.hashCode());
+ }
+
+ @Test
+ public void testNullPropertyValueIsTolerated() {
+ // Storing the map by reference used to tolerate null values; the
defensive copy must too, to
+ // match the sibling TableChange.AddIndex contract.
+ Map<String, String> properties = new HashMap<>();
+ properties.put("key", null);
+
+ Index index =
+ Indexes.of(Index.IndexType.PRIMARY_KEY, "idx", new String[][]
{{"col1"}}, properties);
+
+ Assertions.assertTrue(index.properties().containsKey("key"));
+ Assertions.assertNull(index.properties().get("key"));
+
+ // Still an immutable snapshot: neither the returned map nor later caller
mutations leak.
+ Assertions.assertThrows(
+ UnsupportedOperationException.class, () ->
index.properties().put("k2", "v2"));
+ properties.put("added", "later");
+ Assertions.assertFalse(index.properties().containsKey("added"));
+ }
+}
diff --git
a/catalogs/catalog-lakehouse-paimon/src/test/java/org/apache/gravitino/catalog/lakehouse/paimon/TestGravitinoPaimonTable.java
b/catalogs/catalog-lakehouse-paimon/src/test/java/org/apache/gravitino/catalog/lakehouse/paimon/TestGravitinoPaimonTable.java
index 23c2666054..87a7b72efc 100644
---
a/catalogs/catalog-lakehouse-paimon/src/test/java/org/apache/gravitino/catalog/lakehouse/paimon/TestGravitinoPaimonTable.java
+++
b/catalogs/catalog-lakehouse-paimon/src/test/java/org/apache/gravitino/catalog/lakehouse/paimon/TestGravitinoPaimonTable.java
@@ -313,7 +313,7 @@ public class TestGravitinoPaimonTable {
for (int i = 0; i < indexes.length; i++) {
Assertions.assertEquals(indexes[i].name(), table.index()[i].name());
Assertions.assertEquals(indexes[i].type(), table.index()[i].type());
- Assertions.assertEquals(indexes[i].fieldNames(),
table.index()[i].fieldNames());
+ Assertions.assertArrayEquals(indexes[i].fieldNames(),
table.index()[i].fieldNames());
}
Table loadedTable = paimonCatalogOperations.loadTable(tableIdentifier);
