Introduce AttributeMap interface#5139
Conversation
This change introduces `AttributeMap`, a map-like interface with type-safe key-value pairs. This change only introduces the new interface, but it is currently unused. Future work will adapt the following areas to the new `AttributeMap` interface: - Principal attributes - Event attributes
|
This is a follow-up work discussed here: |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new typed, map-like AttributeMap API in polaris-core (plus mutable/immutable implementations and tests), intended to support future migrations of principal and event attributes away from raw Map<String, Object>.
Changes:
- Added
AttributeMapsealed interface with typedAttributeKey<T>/Attribute<T>and core read/write operations. - Added
AbstractAttributeMap(HashMap-backed) plusMutableAttributeMapandImmutableAttributeMapimplementations (with builders). - Added shared and implementation-specific JUnit tests for read paths, builder behavior, and (im)mutability.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| polaris-core/src/main/java/org/apache/polaris/core/collection/AttributeMap.java | New public API for typed attribute maps, including keys/attributes and core operations. |
| polaris-core/src/main/java/org/apache/polaris/core/collection/AbstractAttributeMap.java | Base HashMap-backed implementation and shared builder logic. |
| polaris-core/src/main/java/org/apache/polaris/core/collection/MutableAttributeMap.java | Mutable implementation and builder. |
| polaris-core/src/main/java/org/apache/polaris/core/collection/ImmutableAttributeMap.java | Immutable implementation with unmodifiable views and mutation guards. |
| polaris-core/src/test/java/org/apache/polaris/core/collection/AbstractAttributeMapTest.java | Shared behavior tests for both implementations. |
| polaris-core/src/test/java/org/apache/polaris/core/collection/MutableAttributeMapTest.java | Mutable-specific mutation/view tests. |
| polaris-core/src/test/java/org/apache/polaris/core/collection/ImmutableAttributeMapTest.java | Immutable-specific immutability tests. |
| public abstract M build(); | ||
| } | ||
|
|
||
| private final Map<AttributeKey<?>, @Nullable Object> delegate = new HashMap<>(); |
There was a problem hiding this comment.
nit: why delegate? This class constructs and owns this piece of data 🤔
There was a problem hiding this comment.
I named it delegate since this class is also a map, but delegates actual storage to another map.
| protected AbstractAttributeMap() {} | ||
|
|
||
| protected AbstractAttributeMap(AttributeMap toCopy) { | ||
| toCopy.entrySet().forEach(entry -> delegate.put(entry.key(), entry.value())); |
There was a problem hiding this comment.
nit: we could probably add from() to the Builder and make constructors just take the map from the builder without re-copying... WDYT?
There was a problem hiding this comment.
The builders already have a "from" method, but it's called putAll currently.
As for changing the copy constructors: I prefer to keep the defensive copy in place, that's safer in the general case imho (the constructors are public).
| @SuppressWarnings("unchecked") | ||
| @Nullable | ||
| private static <T> T cast(@Nullable Object o) { | ||
| return (T) o; |
There was a problem hiding this comment.
Could we require AttributeKey to contain a Class<T> and then use Class.cast()?
There was a problem hiding this comment.
I played with the idea. But it won't buy us much. Only non-generic attribute keys could expose a Class<T> property, others (e.g. List<String>) simply cannot expose such a property. Even using the "type token" idiom wouldn't make that any better: you would be able to expose the attribute's type as a ParameterizedType property, but you still wouldn't be able to use that property to perform a "safe" cast (there is no ParameterizedType.cast() method).
That's why the idea here is to move the invariants to the API: in AttributeMap, there is no way for a writer to add an attribute with a wrong type and make a reader fail with a ClassCastException. It is therefore OK to just do an unsafe cast when reading the attribute's value back.
(well, in fact there is one way to break it: if a writer purposely adds two attributes with the same string key and different types, that would break readers – but that should be seen as an API abuse/misuse.)
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
This change introduces
AttributeMap, a map-like interface with type-safe key-value pairs.This change only introduces the new interface, but it is currently unused.
Future work will adapt the following areas to the new
AttributeMapinterface:Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)