Copilot commented on code in PR #5139: URL: https://github.com/apache/polaris/pull/5139#discussion_r3645366648
########## polaris-core/src/main/java/org/apache/polaris/core/collection/MutableAttributeMap.java: ########## @@ -0,0 +1,61 @@ +/* + * 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.collection; + +/** + * A mutable, type-safe {@link AttributeMap}. Mutable attribute maps are not thread-safe and may + * throw {@link java.util.ConcurrentModificationException} if modified by two threads concurrently. + */ +public final class MutableAttributeMap extends AbstractAttributeMap implements AttributeMap { + + /** Fluent builder for {@link MutableAttributeMap}. */ + public static class Builder + extends AbstractAttributeMap.Builder<MutableAttributeMap, MutableAttributeMap.Builder> { + + @Override + protected MutableAttributeMap.Builder self() { + return this; + } + + @Override + public MutableAttributeMap build() { + return map; + } Review Comment: `MutableAttributeMap.Builder.build()` returns the builder's internal `map` instance, so reusing the builder after `build()` will mutate the previously-built map. This can lead to surprising aliasing bugs (e.g., building twice returns the same object). Consider returning a defensive copy instead. ########## polaris-core/src/main/java/org/apache/polaris/core/collection/AttributeMap.java: ########## @@ -0,0 +1,207 @@ +/* + * 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.collection; + +import java.util.Collection; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.function.Consumer; +import org.jspecify.annotations.NonNull; +import org.jspecify.annotations.Nullable; + +/** + * A map-like structure containing typed attributes, keyed by {@link AttributeKey}. + * + * <p>Unlike a raw {@code Map<String, Object>}, attribute maps guarantee that, for any attribute + * key, the corresponding value has the right type, eliminating unchecked casts in callers. + * + * <p>Note: this guarantee is only valid as long as callers never create two instances of {@link + * AttributeKey} having the same string key, but different types. + * + * <p>Two implementations are available: {@link MutableAttributeMap} and {@link + * ImmutableAttributeMap}. + */ +public sealed interface AttributeMap + permits AbstractAttributeMap, ImmutableAttributeMap, MutableAttributeMap { + + /** A shared empty, immutable instance. */ + ImmutableAttributeMap EMPTY = new ImmutableAttributeMap(); + + /** Returns an immutable copy of {@code map}. */ + static ImmutableAttributeMap copyOf(AttributeMap map) { + return map instanceof ImmutableAttributeMap immutable + ? immutable + : new ImmutableAttributeMap(map); + } + + /** Returns an immutable map containing the given attributes. */ + static ImmutableAttributeMap of(Attribute<?> attribute, Attribute<?>... attributes) { + ImmutableAttributeMap.Builder builder = ImmutableAttributeMap.builder().put(attribute); + for (Attribute<?> attr : attributes) { + builder.put(attr); + } + return builder.build(); + } + + /** + * A typed attribute key. + * + * @param <T> the type of the value associated with this key + * @param key the string identifier for this attribute key. + */ + @SuppressWarnings({"UnusedTypeParameter", "unused"}) + record AttributeKey<T>(String key) { + + @Override + @NonNull + public String toString() { + return key; + } + } + + /** + * An entry associating an {@link AttributeKey} with a value of its type. Used to iterate over + * attributes in the map. + * + * @see #entrySet() + * @see #forEach(Consumer) + * @see #put(Attribute) + */ + record Attribute<T>(AttributeKey<T> key, T value) { + + @Override + @NonNull + public String toString() { + // Do not print attribute values! + return "Attribute{key=" + key + "}"; + } + } + + /** Returns the number of key-value mappings in this map. */ + int size(); + + /** Returns {@code true} if this map contains no key-value mappings. */ + boolean isEmpty(); + + /** Returns the value associated with {@code key}, or {@code null} if absent. */ + @Nullable <T> T get(AttributeKey<T> key); + + /** + * Returns the value for {@code key} if present, otherwise {@code defaultValue}. + * + * <p>Unlike {@link #get}, this distinguishes between a missing key and a key explicitly mapped to + * {@code null}. + */ + @Nullable <V> V getOrDefault(AttributeKey<V> key, @Nullable V defaultValue); + + /** + * Returns the value associated with {@code key} wrapped in an {@link Optional}, or an empty + * {@link Optional} if the key is absent. + */ + default <T> Optional<T> getOptional(AttributeKey<T> key) { + return Optional.ofNullable(get(key)); + } + + /** + * Returns the value associated with {@code key}, throwing {@link IllegalStateException} if the + * key is absent. + */ + default <T> T getRequired(AttributeKey<T> key) { + if (!containsKey(key)) { + throw new IllegalStateException("Required attribute " + key.key() + " not found"); + } + return get(key); + } + + /** + * Associates the specified value with the specified key in this map. Returns the previous value + * associated with {@code key}, or {@code null} if there was no mapping for {@code key}. + */ + @Nullable <T> T put(AttributeKey<T> key, @Nullable T value); + + /** + * Adds the given attribute to this map. Returns the previous value associated with the + * attribute's key, or {@code null} if there was no mapping for that key. This is just a shortcut + * for {@code putAttribute(attribute.getKey(), attribute.getValue())}. + */ Review Comment: Javadoc references `putAttribute(attribute.getKey(), attribute.getValue())`, but those methods don't exist (the record accessors are `key()` / `value()` and the method is `put`). This makes the API docs misleading. ########## polaris-core/src/main/java/org/apache/polaris/core/collection/AttributeMap.java: ########## @@ -0,0 +1,207 @@ +/* + * 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.collection; + +import java.util.Collection; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.function.Consumer; +import org.jspecify.annotations.NonNull; +import org.jspecify.annotations.Nullable; + +/** + * A map-like structure containing typed attributes, keyed by {@link AttributeKey}. + * + * <p>Unlike a raw {@code Map<String, Object>}, attribute maps guarantee that, for any attribute + * key, the corresponding value has the right type, eliminating unchecked casts in callers. + * + * <p>Note: this guarantee is only valid as long as callers never create two instances of {@link + * AttributeKey} having the same string key, but different types. + * + * <p>Two implementations are available: {@link MutableAttributeMap} and {@link + * ImmutableAttributeMap}. + */ +public sealed interface AttributeMap + permits AbstractAttributeMap, ImmutableAttributeMap, MutableAttributeMap { + + /** A shared empty, immutable instance. */ + ImmutableAttributeMap EMPTY = new ImmutableAttributeMap(); + + /** Returns an immutable copy of {@code map}. */ + static ImmutableAttributeMap copyOf(AttributeMap map) { + return map instanceof ImmutableAttributeMap immutable + ? immutable + : new ImmutableAttributeMap(map); + } + + /** Returns an immutable map containing the given attributes. */ + static ImmutableAttributeMap of(Attribute<?> attribute, Attribute<?>... attributes) { + ImmutableAttributeMap.Builder builder = ImmutableAttributeMap.builder().put(attribute); + for (Attribute<?> attr : attributes) { + builder.put(attr); + } + return builder.build(); + } + + /** + * A typed attribute key. + * + * @param <T> the type of the value associated with this key + * @param key the string identifier for this attribute key. + */ + @SuppressWarnings({"UnusedTypeParameter", "unused"}) + record AttributeKey<T>(String key) { + + @Override + @NonNull + public String toString() { + return key; + } + } + + /** + * An entry associating an {@link AttributeKey} with a value of its type. Used to iterate over + * attributes in the map. + * + * @see #entrySet() + * @see #forEach(Consumer) + * @see #put(Attribute) + */ + record Attribute<T>(AttributeKey<T> key, T value) { + + @Override + @NonNull + public String toString() { + // Do not print attribute values! + return "Attribute{key=" + key + "}"; + } + } + + /** Returns the number of key-value mappings in this map. */ + int size(); + + /** Returns {@code true} if this map contains no key-value mappings. */ + boolean isEmpty(); + + /** Returns the value associated with {@code key}, or {@code null} if absent. */ + @Nullable <T> T get(AttributeKey<T> key); + + /** + * Returns the value for {@code key} if present, otherwise {@code defaultValue}. + * + * <p>Unlike {@link #get}, this distinguishes between a missing key and a key explicitly mapped to + * {@code null}. + */ + @Nullable <V> V getOrDefault(AttributeKey<V> key, @Nullable V defaultValue); + + /** + * Returns the value associated with {@code key} wrapped in an {@link Optional}, or an empty + * {@link Optional} if the key is absent. + */ + default <T> Optional<T> getOptional(AttributeKey<T> key) { + return Optional.ofNullable(get(key)); + } + + /** + * Returns the value associated with {@code key}, throwing {@link IllegalStateException} if the + * key is absent. + */ + default <T> T getRequired(AttributeKey<T> key) { + if (!containsKey(key)) { + throw new IllegalStateException("Required attribute " + key.key() + " not found"); + } + return get(key); + } + + /** + * Associates the specified value with the specified key in this map. Returns the previous value + * associated with {@code key}, or {@code null} if there was no mapping for {@code key}. + */ + @Nullable <T> T put(AttributeKey<T> key, @Nullable T value); + + /** + * Adds the given attribute to this map. Returns the previous value associated with the + * attribute's key, or {@code null} if there was no mapping for that key. This is just a shortcut + * for {@code putAttribute(attribute.getKey(), attribute.getValue())}. + */ + @Nullable + default <T> T put(Attribute<T> attribute) { + return put(attribute.key(), attribute.value()); + } + + /** Copies all the attributes from the specified map to this map. */ + void putAll(AttributeMap map); + + /** + * Removes the mapping for the given typed {@code key}, if present. Returns the previous value + * associated with {@code key}, or {@code null} if there was no mapping for {@code key}. + */ + @Nullable <T> T remove(AttributeKey<T> key); + + /** Returns {@code true} if this map contains an attribute with the specified {@code key}. */ + boolean containsKey(AttributeKey<?> key); + + /** Returns {@code true} if this map contains an attribute with the specified {@code value} */ + boolean containsValue(@Nullable Object value); + + /** + * Returns a {@link Set} view of the keys contained in this map. The set is backed by the map, so + * changes to the map are reflected in the set, and vice versa. If the map is modified while an + * iteration over the set is in progress (except through the iterator's own {@code remove} + * operation), the results of the iteration are undefined. The set supports element removal, which + * removes the corresponding mapping from the map, via the {@code Iterator.remove}, {@code + * Set.remove}, {@code removeAll}, {@code retainAll}, and {@code clear} operations. It does not + * support the {@code add} or {@code addAll} operations. + */ + Set<AttributeKey<?>> keySet(); + + /** + * Returns a {@link Collection} view of the values contained in this map. The collection is backed + * by the map, so changes to the map are reflected in the collection, and vice versa. If the map + * is modified while an iteration over the collection is in progress (except through the + * iterator's own {@code remove} operation), the results of the iteration are undefined. The + * collection supports element removal, which removes the corresponding mapping from the map, via + * the {@code Iterator.remove}, {@code Collection.remove}, {@code removeAll}, {@code retainAll} + * and {@code clear} operations. It does not support the {@code add} or {@code addAll} operations. + */ Review Comment: `values()` Javadoc promises a mutable, map-backed collection that supports removals, but `ImmutableAttributeMap.values()` returns an unmodifiable view (tests assert mutations throw). Please adjust the contract to cover immutable implementations returning unmodifiable views. ########## polaris-core/src/main/java/org/apache/polaris/core/collection/AttributeMap.java: ########## @@ -0,0 +1,207 @@ +/* + * 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.collection; + +import java.util.Collection; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.function.Consumer; +import org.jspecify.annotations.NonNull; +import org.jspecify.annotations.Nullable; + +/** + * A map-like structure containing typed attributes, keyed by {@link AttributeKey}. + * + * <p>Unlike a raw {@code Map<String, Object>}, attribute maps guarantee that, for any attribute + * key, the corresponding value has the right type, eliminating unchecked casts in callers. + * + * <p>Note: this guarantee is only valid as long as callers never create two instances of {@link + * AttributeKey} having the same string key, but different types. + * + * <p>Two implementations are available: {@link MutableAttributeMap} and {@link + * ImmutableAttributeMap}. + */ +public sealed interface AttributeMap + permits AbstractAttributeMap, ImmutableAttributeMap, MutableAttributeMap { + + /** A shared empty, immutable instance. */ + ImmutableAttributeMap EMPTY = new ImmutableAttributeMap(); + + /** Returns an immutable copy of {@code map}. */ + static ImmutableAttributeMap copyOf(AttributeMap map) { + return map instanceof ImmutableAttributeMap immutable + ? immutable + : new ImmutableAttributeMap(map); + } + + /** Returns an immutable map containing the given attributes. */ + static ImmutableAttributeMap of(Attribute<?> attribute, Attribute<?>... attributes) { + ImmutableAttributeMap.Builder builder = ImmutableAttributeMap.builder().put(attribute); + for (Attribute<?> attr : attributes) { + builder.put(attr); + } + return builder.build(); + } + + /** + * A typed attribute key. + * + * @param <T> the type of the value associated with this key + * @param key the string identifier for this attribute key. + */ + @SuppressWarnings({"UnusedTypeParameter", "unused"}) + record AttributeKey<T>(String key) { + + @Override + @NonNull + public String toString() { + return key; + } + } + + /** + * An entry associating an {@link AttributeKey} with a value of its type. Used to iterate over + * attributes in the map. + * + * @see #entrySet() + * @see #forEach(Consumer) + * @see #put(Attribute) + */ + record Attribute<T>(AttributeKey<T> key, T value) { + + @Override + @NonNull + public String toString() { + // Do not print attribute values! + return "Attribute{key=" + key + "}"; + } + } + + /** Returns the number of key-value mappings in this map. */ + int size(); + + /** Returns {@code true} if this map contains no key-value mappings. */ + boolean isEmpty(); + + /** Returns the value associated with {@code key}, or {@code null} if absent. */ + @Nullable <T> T get(AttributeKey<T> key); + + /** + * Returns the value for {@code key} if present, otherwise {@code defaultValue}. + * + * <p>Unlike {@link #get}, this distinguishes between a missing key and a key explicitly mapped to + * {@code null}. + */ + @Nullable <V> V getOrDefault(AttributeKey<V> key, @Nullable V defaultValue); + + /** + * Returns the value associated with {@code key} wrapped in an {@link Optional}, or an empty + * {@link Optional} if the key is absent. + */ + default <T> Optional<T> getOptional(AttributeKey<T> key) { + return Optional.ofNullable(get(key)); + } + + /** + * Returns the value associated with {@code key}, throwing {@link IllegalStateException} if the + * key is absent. + */ + default <T> T getRequired(AttributeKey<T> key) { + if (!containsKey(key)) { + throw new IllegalStateException("Required attribute " + key.key() + " not found"); + } + return get(key); + } + + /** + * Associates the specified value with the specified key in this map. Returns the previous value + * associated with {@code key}, or {@code null} if there was no mapping for {@code key}. + */ + @Nullable <T> T put(AttributeKey<T> key, @Nullable T value); + + /** + * Adds the given attribute to this map. Returns the previous value associated with the + * attribute's key, or {@code null} if there was no mapping for that key. This is just a shortcut + * for {@code putAttribute(attribute.getKey(), attribute.getValue())}. + */ + @Nullable + default <T> T put(Attribute<T> attribute) { + return put(attribute.key(), attribute.value()); + } + + /** Copies all the attributes from the specified map to this map. */ + void putAll(AttributeMap map); + + /** + * Removes the mapping for the given typed {@code key}, if present. Returns the previous value + * associated with {@code key}, or {@code null} if there was no mapping for {@code key}. + */ + @Nullable <T> T remove(AttributeKey<T> key); + + /** Returns {@code true} if this map contains an attribute with the specified {@code key}. */ + boolean containsKey(AttributeKey<?> key); + + /** Returns {@code true} if this map contains an attribute with the specified {@code value} */ + boolean containsValue(@Nullable Object value); + + /** + * Returns a {@link Set} view of the keys contained in this map. The set is backed by the map, so + * changes to the map are reflected in the set, and vice versa. If the map is modified while an + * iteration over the set is in progress (except through the iterator's own {@code remove} + * operation), the results of the iteration are undefined. The set supports element removal, which + * removes the corresponding mapping from the map, via the {@code Iterator.remove}, {@code + * Set.remove}, {@code removeAll}, {@code retainAll}, and {@code clear} operations. It does not + * support the {@code add} or {@code addAll} operations. + */ Review Comment: `keySet()` Javadoc promises a view that supports removals and other mutations, but `ImmutableAttributeMap.keySet()` returns an unmodifiable view (tests assert mutations throw). The contract should reflect that immutable implementations may return unmodifiable views. ########## polaris-core/src/test/java/org/apache/polaris/core/collection/AbstractAttributeMapTest.java: ########## @@ -0,0 +1,234 @@ +/* + * 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.collection; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import org.apache.polaris.core.collection.AttributeMap.Attribute; +import org.apache.polaris.core.collection.AttributeMap.AttributeKey; +import org.junit.jupiter.api.Test; + +/** + * Shared read-path and builder tests executed against both {@link MutableAttributeMap} and {@link + * ImmutableAttributeMap}. + */ +abstract class AbstractAttributeMapTest<T extends AttributeMap> { + + static final AttributeKey<String> KEY1 = new AttributeKey<>("attr1"); + static final AttributeKey<Integer> KEY2 = new AttributeKey<>("attr2"); + static final AttributeKey<List<Boolean>> KEY3 = new AttributeKey<>("attr3"); + + static final String VALUE1 = "1"; + static final Integer VALUE2 = null; + static final List<Boolean> VALUE3 = List.of(true, false); + + static final Attribute<String> ATTR1 = new Attribute<>(KEY1, VALUE1); + static final Attribute<Integer> ATTR2 = new Attribute<>(KEY2, VALUE2); + static final Attribute<List<Boolean>> ATTR3 = new Attribute<>(KEY3, VALUE3); + + static final AttributeKey<String> NONEXISTENT_KEY = new AttributeKey<>("nonexistent"); + + protected abstract AbstractAttributeMap.Builder<T, ?> emptyBuilder(); + + protected T emptyMap() { + return emptyBuilder().build(); + } + + protected T threeEntryMap() { + return emptyBuilder().put(KEY1, VALUE1).put(KEY2, VALUE2).put(KEY3, VALUE3).build(); + } + + @Test + void size() { + assertThat(threeEntryMap().size()).isEqualTo(3); + assertThat(emptyMap().size()).isEqualTo(0); + } + + @Test + void isEmpty() { + assertThat(threeEntryMap().isEmpty()).isFalse(); + assertThat(emptyMap().isEmpty()).isTrue(); + } + + @Test + void get() { + AttributeMap map = threeEntryMap(); + assertThat(map.get(KEY1)).isEqualTo(VALUE1); + assertThat(map.get(KEY2)).isEqualTo(VALUE2); + assertThat(map.get(KEY3)).isEqualTo(VALUE3); + assertThat(map.get(NONEXISTENT_KEY)).isNull(); + } + + @Test + void getOrDefault() { + AttributeMap map = threeEntryMap(); + assertThat(map.getOrDefault(KEY1, "default")).isEqualTo(VALUE1); + assertThat(map.getOrDefault(KEY2, 0)).isEqualTo(VALUE2); + assertThat(map.getOrDefault(KEY3, List.of(false, true))).isEqualTo(VALUE3); + assertThat(map.getOrDefault(NONEXISTENT_KEY, "default")).isEqualTo("default"); + assertThat(map.getOrDefault(NONEXISTENT_KEY, null)).isNull(); + } + + @Test + void getOptional() { + AttributeMap map = threeEntryMap(); + assertThat(map.getOptional(KEY1)).contains(VALUE1); + assertThat(map.getOptional(KEY2)).isEmpty(); + assertThat(map.getOptional(KEY3)).contains(VALUE3); + assertThat(map.getOptional(NONEXISTENT_KEY)).isEmpty(); + } + + @Test + void getRequired() { + AttributeMap map = threeEntryMap(); + assertThat(map.getRequired(KEY1)).isEqualTo(VALUE1); + assertThat(map.getRequired(KEY2)).isEqualTo(VALUE2); + assertThat(map.getOptional(KEY3)).contains(VALUE3); Review Comment: `getRequired()` test uses `getOptional(KEY3)` instead of `getRequired(KEY3)`, so it doesn't actually exercise the method for the third key and doesn't match the test's intent/name. ########## polaris-core/src/main/java/org/apache/polaris/core/collection/AttributeMap.java: ########## @@ -0,0 +1,207 @@ +/* + * 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.collection; + +import java.util.Collection; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.function.Consumer; +import org.jspecify.annotations.NonNull; +import org.jspecify.annotations.Nullable; + +/** + * A map-like structure containing typed attributes, keyed by {@link AttributeKey}. + * + * <p>Unlike a raw {@code Map<String, Object>}, attribute maps guarantee that, for any attribute + * key, the corresponding value has the right type, eliminating unchecked casts in callers. + * + * <p>Note: this guarantee is only valid as long as callers never create two instances of {@link + * AttributeKey} having the same string key, but different types. + * + * <p>Two implementations are available: {@link MutableAttributeMap} and {@link + * ImmutableAttributeMap}. + */ +public sealed interface AttributeMap + permits AbstractAttributeMap, ImmutableAttributeMap, MutableAttributeMap { + + /** A shared empty, immutable instance. */ + ImmutableAttributeMap EMPTY = new ImmutableAttributeMap(); + + /** Returns an immutable copy of {@code map}. */ + static ImmutableAttributeMap copyOf(AttributeMap map) { + return map instanceof ImmutableAttributeMap immutable + ? immutable + : new ImmutableAttributeMap(map); + } + + /** Returns an immutable map containing the given attributes. */ + static ImmutableAttributeMap of(Attribute<?> attribute, Attribute<?>... attributes) { + ImmutableAttributeMap.Builder builder = ImmutableAttributeMap.builder().put(attribute); + for (Attribute<?> attr : attributes) { + builder.put(attr); + } + return builder.build(); + } + + /** + * A typed attribute key. + * + * @param <T> the type of the value associated with this key + * @param key the string identifier for this attribute key. + */ + @SuppressWarnings({"UnusedTypeParameter", "unused"}) + record AttributeKey<T>(String key) { + + @Override + @NonNull + public String toString() { + return key; + } + } Review Comment: `AttributeKey.toString()` is annotated `@NonNull` but can currently return `null` if a caller constructs `new AttributeKey<>(null)`. To keep the nullness contract consistent (and avoid null keys in the underlying map), consider enforcing a non-null key at construction time and/or annotating the record component as `@NonNull`. ########## polaris-core/src/test/java/org/apache/polaris/core/collection/MutableAttributeMapTest.java: ########## @@ -0,0 +1,165 @@ +/* + * 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.collection; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.Iterator; +import java.util.List; +import org.apache.polaris.core.collection.AttributeMap.Attribute; +import org.apache.polaris.core.collection.AttributeMap.AttributeKey; +import org.junit.jupiter.api.Test; + +class MutableAttributeMapTest extends AbstractAttributeMapTest<MutableAttributeMap> { + + @Override + protected MutableAttributeMap.Builder emptyBuilder() { + return MutableAttributeMap.builder(); + } + + @Test + void copyConstructor() { + AttributeMap original = MutableAttributeMap.builder().put(KEY1, "1").build(); + AttributeMap copy = new ImmutableAttributeMap(original); Review Comment: This test is named `copyConstructor()` but constructs the copy via `new ImmutableAttributeMap(original)` (and `ImmutableAttributeMapTest` already covers that). If the intent is to validate `MutableAttributeMap(AttributeMap)` copy behavior, it should instantiate `MutableAttributeMap` here. ########## polaris-core/src/main/java/org/apache/polaris/core/collection/AttributeMap.java: ########## @@ -0,0 +1,207 @@ +/* + * 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.collection; + +import java.util.Collection; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.function.Consumer; +import org.jspecify.annotations.NonNull; +import org.jspecify.annotations.Nullable; + +/** + * A map-like structure containing typed attributes, keyed by {@link AttributeKey}. + * + * <p>Unlike a raw {@code Map<String, Object>}, attribute maps guarantee that, for any attribute + * key, the corresponding value has the right type, eliminating unchecked casts in callers. + * + * <p>Note: this guarantee is only valid as long as callers never create two instances of {@link + * AttributeKey} having the same string key, but different types. + * + * <p>Two implementations are available: {@link MutableAttributeMap} and {@link + * ImmutableAttributeMap}. + */ +public sealed interface AttributeMap + permits AbstractAttributeMap, ImmutableAttributeMap, MutableAttributeMap { + + /** A shared empty, immutable instance. */ + ImmutableAttributeMap EMPTY = new ImmutableAttributeMap(); + + /** Returns an immutable copy of {@code map}. */ + static ImmutableAttributeMap copyOf(AttributeMap map) { + return map instanceof ImmutableAttributeMap immutable + ? immutable + : new ImmutableAttributeMap(map); + } + + /** Returns an immutable map containing the given attributes. */ + static ImmutableAttributeMap of(Attribute<?> attribute, Attribute<?>... attributes) { + ImmutableAttributeMap.Builder builder = ImmutableAttributeMap.builder().put(attribute); + for (Attribute<?> attr : attributes) { + builder.put(attr); + } + return builder.build(); + } + + /** + * A typed attribute key. + * + * @param <T> the type of the value associated with this key + * @param key the string identifier for this attribute key. + */ + @SuppressWarnings({"UnusedTypeParameter", "unused"}) + record AttributeKey<T>(String key) { + + @Override + @NonNull + public String toString() { + return key; + } + } + + /** + * An entry associating an {@link AttributeKey} with a value of its type. Used to iterate over + * attributes in the map. + * + * @see #entrySet() + * @see #forEach(Consumer) + * @see #put(Attribute) + */ + record Attribute<T>(AttributeKey<T> key, T value) { + + @Override + @NonNull + public String toString() { + // Do not print attribute values! + return "Attribute{key=" + key + "}"; + } + } + + /** Returns the number of key-value mappings in this map. */ + int size(); + + /** Returns {@code true} if this map contains no key-value mappings. */ + boolean isEmpty(); + + /** Returns the value associated with {@code key}, or {@code null} if absent. */ + @Nullable <T> T get(AttributeKey<T> key); + + /** + * Returns the value for {@code key} if present, otherwise {@code defaultValue}. + * + * <p>Unlike {@link #get}, this distinguishes between a missing key and a key explicitly mapped to + * {@code null}. + */ + @Nullable <V> V getOrDefault(AttributeKey<V> key, @Nullable V defaultValue); + + /** + * Returns the value associated with {@code key} wrapped in an {@link Optional}, or an empty + * {@link Optional} if the key is absent. + */ + default <T> Optional<T> getOptional(AttributeKey<T> key) { + return Optional.ofNullable(get(key)); + } + + /** + * Returns the value associated with {@code key}, throwing {@link IllegalStateException} if the + * key is absent. + */ + default <T> T getRequired(AttributeKey<T> key) { + if (!containsKey(key)) { + throw new IllegalStateException("Required attribute " + key.key() + " not found"); + } + return get(key); + } + + /** + * Associates the specified value with the specified key in this map. Returns the previous value + * associated with {@code key}, or {@code null} if there was no mapping for {@code key}. + */ + @Nullable <T> T put(AttributeKey<T> key, @Nullable T value); + + /** + * Adds the given attribute to this map. Returns the previous value associated with the + * attribute's key, or {@code null} if there was no mapping for that key. This is just a shortcut + * for {@code putAttribute(attribute.getKey(), attribute.getValue())}. + */ + @Nullable + default <T> T put(Attribute<T> attribute) { + return put(attribute.key(), attribute.value()); + } + + /** Copies all the attributes from the specified map to this map. */ + void putAll(AttributeMap map); + + /** + * Removes the mapping for the given typed {@code key}, if present. Returns the previous value + * associated with {@code key}, or {@code null} if there was no mapping for {@code key}. + */ + @Nullable <T> T remove(AttributeKey<T> key); + + /** Returns {@code true} if this map contains an attribute with the specified {@code key}. */ + boolean containsKey(AttributeKey<?> key); + + /** Returns {@code true} if this map contains an attribute with the specified {@code value} */ + boolean containsValue(@Nullable Object value); + + /** + * Returns a {@link Set} view of the keys contained in this map. The set is backed by the map, so + * changes to the map are reflected in the set, and vice versa. If the map is modified while an + * iteration over the set is in progress (except through the iterator's own {@code remove} + * operation), the results of the iteration are undefined. The set supports element removal, which + * removes the corresponding mapping from the map, via the {@code Iterator.remove}, {@code + * Set.remove}, {@code removeAll}, {@code retainAll}, and {@code clear} operations. It does not + * support the {@code add} or {@code addAll} operations. + */ + Set<AttributeKey<?>> keySet(); + + /** + * Returns a {@link Collection} view of the values contained in this map. The collection is backed + * by the map, so changes to the map are reflected in the collection, and vice versa. If the map + * is modified while an iteration over the collection is in progress (except through the + * iterator's own {@code remove} operation), the results of the iteration are undefined. The + * collection supports element removal, which removes the corresponding mapping from the map, via + * the {@code Iterator.remove}, {@code Collection.remove}, {@code removeAll}, {@code retainAll} + * and {@code clear} operations. It does not support the {@code add} or {@code addAll} operations. + */ + Collection<Object> values(); + + /** + * Returns a {@link Set} view of the mappings contained in this map. The set is backed by the map, + * so changes to the map are reflected in the set, and vice versa. If the map is modified while an + * iteration over the set is in progress (except through the iterator's own {@code remove} + * operation, or through the {@code setValue} operation on a map entry returned by the iterator) + * the results of the iteration are undefined. The set supports element removal, which removes the + * corresponding mapping from the map, via the {@code Iterator.remove}, {@code Set.remove}, {@code + * removeAll}, {@code retainAll} and {@code clear} operations. It does not support the {@code add} + * or {@code addAll} operations. + */ Review Comment: `entrySet()` Javadoc refers to `setValue` on a "map entry", but this API returns `Attribute` records (no `setValue`), and `ImmutableAttributeMap.entrySet()` is unmodifiable. The contract should avoid `Map.Entry`-specific language and reflect immutable behavior. -- 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]
