jbonofre commented on code in PR #1055: URL: https://github.com/apache/polaris/pull/1055#discussion_r1967237371
########## polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntity.java: ########## @@ -25,12 +25,23 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Optional; import java.util.function.Predicate; import java.util.stream.Collectors; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +/** + * For legacy reasons, this class is only a thin facade over PolarisBaseEntity's members/methods. No + * direct members should be added to this class; rather, they should reside in the PolarisBaseEntity + * and this class should just contain the relevant builder methods, etc. The intention when using + * this class is to use "immutable" semantics as much as possible, for example constructing new + * copies with the Builder pattern when "mutating" fields rather than ever chaing fields in-place. + * Currently, code that intends to operate directly on a PolarisBaseEntity may not adhere to + * immutability semantics, and may modify the entity in-place. + * + * <p>TODO: Combine this fully into PolarisBaseEntity, refactor all callsites to use strict Review Comment: It sounds reasonnable to me to only have `PolarisBaseEntity`. `PolarisEntity` is a bit confusing at first glance. -- 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]
