[#13230] fix(api): defensively copy fieldNames and properties in IndexImpl

[LuciferYang]

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.

[Copilot]

🟡 Changes recommended

Null-handling compatibility regressions remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request makes IndexImpl immutable through defensive copying and adds regression tests.

Changes:

• Deep-copy field-name arrays.
• Snapshot index properties.
• Test mutation isolation.

File summaries

File	Description
api/src/test/java/org/apache/gravitino/rel/indexes/TestIndexes.java	Tests mutation isolation.
api/src/main/java/org/apache/gravitino/rel/indexes/Indexes.java	Implements defensive copying.

Review details

Suppressed comments (2)

api/src/main/java/org/apache/gravitino/rel/indexes/Indexes.java:164

• ImmutableMap.copyOf rejects null keys and values. Before this change, Indexes.of accepted a Map<String, String> containing null entries, and the API does not document a null-entry restriction; such inputs now fail with an NPE during construction despite the stated no-API-change behavior. Snapshot with an unmodifiable mutable-map copy (as TableChange.AddIndex does at api/src/main/java/org/apache/gravitino/rel/TableChange.java:790-793) or explicitly validate and document the new restriction.

      this.properties = properties == null ? ImmutableMap.of() : ImmutableMap.copyOf(properties);

api/src/main/java/org/apache/gravitino/rel/indexes/Indexes.java:190

• The accessor repeats the same unconditional clone, so an index containing a null field-name row would still throw NullPointerException when read. Apply the same null-preserving copy here so fieldNames() does not introduce a new failure mode.

          : Arrays.stream(fieldNames).map(String[]::clone).toArray(String[][]::new);

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[LuciferYang]

Addressed in 206060b. The field-name deep copy now preserves null rows (row == null ? null : row.clone()) in both the constructor and fieldNames(), and properties is stored as a null-tolerant unmodifiableMap(new HashMap<>(...)) copy, matching the sibling TableChange.AddIndex. A null field-name row or a null property value no longer throws, and the built index stays externally immutable. Added regression tests for both.