From 8e2cfbdaed5fd773b6b3f7f891a837afaeec8c3e Mon Sep 17 00:00:00 2001 From: iting0321 Date: Sun, 5 Jul 2026 20:42:55 +0900 Subject: [PATCH 1/3] ensure TreeMapMetaStore range reads return copies --- .../transactional/TreeMapMetaStore.java | 15 ++-- .../transactional/TreeMapMetaStoreTest.java | 72 +++++++++++++++++++ 2 files changed, 83 insertions(+), 4 deletions(-) create mode 100644 polaris-core/src/test/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStoreTest.java diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStore.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStore.java index 2110283e6b8..f31d4c3594e 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStore.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStore.java @@ -20,6 +20,7 @@ import jakarta.annotation.Nonnull; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.TreeMap; import java.util.concurrent.atomic.AtomicLong; @@ -63,8 +64,6 @@ public String buildKey(T value) { /** * read a value in the slice, will return null if not found * - *

TODO: return a copy of each object to avoid mutating the records - * * @param key key for that value */ public T read(String key) { @@ -81,7 +80,7 @@ public T read(String key) { public List readRange(String prefix) { ensureReadTr(); if (prefix.isEmpty()) { - return new ArrayList<>(this.slice.values()); + return copyValues(this.slice.values()); } // end of the key @@ -90,7 +89,15 @@ public List readRange(String prefix) { + (char) (prefix.charAt(prefix.length() - 1) + 1); // Get the sub-map with keys in the range [prefix, endKey) - return new ArrayList<>(slice.subMap(prefix, true, endKey, false).values()); + return copyValues(slice.subMap(prefix, true, endKey, false).values()); + } + + private List copyValues(Collection values) { + List copied = new ArrayList<>(values.size()); + for (T value : values) { + copied.add(this.copyRecord.apply(value)); + } + return copied; } /** diff --git a/polaris-core/src/test/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStoreTest.java b/polaris-core/src/test/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStoreTest.java new file mode 100644 index 00000000000..bc2f6983aa8 --- /dev/null +++ b/polaris-core/src/test/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStoreTest.java @@ -0,0 +1,72 @@ +/* + * 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.polaris.core.persistence.transactional; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.List; +import org.apache.polaris.core.PolarisDefaultDiagServiceImpl; +import org.apache.polaris.core.PolarisDiagnostics; +import org.apache.polaris.core.entity.PolarisGrantRecord; +import org.junit.jupiter.api.Test; + +class TreeMapMetaStoreTest { + + private final PolarisDiagnostics diagnostics = new PolarisDefaultDiagServiceImpl(); + + @Test + void readRangeReturnsCopiesForEmptyPrefix() { + TreeMapMetaStore store = new TreeMapMetaStore(diagnostics); + PolarisGrantRecord grantRecord = new PolarisGrantRecord(1L, 2L, 3L, 4L, 5); + + store.runActionInTransaction( + diagnostics, () -> store.getSliceGrantRecords().write(grantRecord)); + + List range = + store.runInReadTransaction(diagnostics, () -> store.getSliceGrantRecords().readRange("")); + range.get(0).setPrivilegeCode(99); + + PolarisGrantRecord stored = + store.runInReadTransaction( + diagnostics, () -> store.getSliceGrantRecords().readRange("").get(0)); + + assertThat(stored.getPrivilegeCode()).isEqualTo(5); + } + + @Test + void readRangeReturnsCopiesForNonEmptyPrefix() { + TreeMapMetaStore store = new TreeMapMetaStore(diagnostics); + PolarisGrantRecord grantRecord = new PolarisGrantRecord(1L, 2L, 3L, 4L, 5); + String prefix = store.buildPrefixKeyComposite(1L, 2L); + + store.runActionInTransaction( + diagnostics, () -> store.getSliceGrantRecords().write(grantRecord)); + + List range = + store.runInReadTransaction( + diagnostics, () -> store.getSliceGrantRecords().readRange(prefix)); + range.get(0).setPrivilegeCode(99); + + PolarisGrantRecord stored = + store.runInReadTransaction( + diagnostics, () -> store.getSliceGrantRecords().readRange(prefix).get(0)); + + assertThat(stored.getPrivilegeCode()).isEqualTo(5); + } +} From 27fffe33d9c76272670b3d85cf11c875813585de Mon Sep 17 00:00:00 2001 From: iting0321 Date: Sun, 12 Jul 2026 22:46:14 +0900 Subject: [PATCH 2/3] avoid copying records during TreeMapMetaStore range deletes --- .../transactional/TreeMapMetaStore.java | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStore.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStore.java index f31d4c3594e..b32c22b3ee4 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStore.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStore.java @@ -100,6 +100,17 @@ private List copyValues(Collection values) { return copied; } + private List copyKeysInRange(String prefix) { + if (prefix.isEmpty()) { + return new ArrayList<>(this.slice.keySet()); + } + + String endKey = + prefix.substring(0, prefix.length() - 1) + + (char) (prefix.charAt(prefix.length() - 1) + 1); + return new ArrayList<>(slice.subMap(prefix, true, endKey, false).keySet()); + } + /** * write a value in the slice * @@ -139,9 +150,9 @@ public void delete(String key) { */ public void deleteRange(String prefix) { ensureReadWriteTr(); - List elements = this.readRange(prefix); - for (T element : elements) { - this.delete(element); + List keys = this.copyKeysInRange(prefix); + for (String key : keys) { + this.delete(key); } } From 8a12db170215ca41ef552a4eb7e8a28006452cba Mon Sep 17 00:00:00 2001 From: iting0321 Date: Wed, 15 Jul 2026 13:02:40 +0800 Subject: [PATCH 3/3] refactor TreeMap range end key computation --- .../transactional/TreeMapMetaStore.java | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStore.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStore.java index b32c22b3ee4..be5fbbe4fc2 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStore.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapMetaStore.java @@ -83,13 +83,8 @@ public List readRange(String prefix) { return copyValues(this.slice.values()); } - // end of the key - String endKey = - prefix.substring(0, prefix.length() - 1) - + (char) (prefix.charAt(prefix.length() - 1) + 1); - - // Get the sub-map with keys in the range [prefix, endKey) - return copyValues(slice.subMap(prefix, true, endKey, false).values()); + // Get the sub-map with keys in the range [prefix, rangeEndKey(prefix)) + return copyValues(slice.subMap(prefix, true, rangeEndKey(prefix), false).values()); } private List copyValues(Collection values) { @@ -105,10 +100,12 @@ private List copyKeysInRange(String prefix) { return new ArrayList<>(this.slice.keySet()); } - String endKey = - prefix.substring(0, prefix.length() - 1) - + (char) (prefix.charAt(prefix.length() - 1) + 1); - return new ArrayList<>(slice.subMap(prefix, true, endKey, false).keySet()); + return new ArrayList<>(slice.subMap(prefix, true, rangeEndKey(prefix), false).keySet()); + } + + private String rangeEndKey(String prefix) { + return prefix.substring(0, prefix.length() - 1) + + (char) (prefix.charAt(prefix.length() - 1) + 1); } /**