XJDKC commented on code in PR #1899: URL: https://github.com/apache/polaris/pull/1899#discussion_r2151010447
########## polaris-core/src/main/java/org/apache/polaris/core/entity/transformation/EntityTransformationEngine.java: ########## @@ -0,0 +1,44 @@ +/* + * 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.entity.transformation; + +import org.apache.polaris.core.entity.PolarisBaseEntity; + +/** + * Engine responsible for applying a sequence of {@link EntityTransformer} transformations to a + * {@link PolarisBaseEntity}. + * + * <p>This abstraction allows Polaris to customize or enrich entities during runtime or persistence, + * based on configured or contextual logic (e.g., injecting service identity info, computing derived + * fields). + */ +public interface EntityTransformationEngine { Review Comment: > It's unfortunate that we haven't had a chance yet to make PolarisBaseEntity immutable, but maybe our naming for this set of classes should be something that more strongly implies producing a "new transformed copy of the entity" rather than make it tempting for anyone to try mutating them in-place. > > What does everyone think of naming these instead: > > EntityTransformationEngine/EntityTransformer/TransformationPoint EntityDecorationEngine/EntityDecorator/DecorationPoint ? ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java: ########## @@ -477,6 +479,14 @@ private void revokeGrantRecord( ms.persistStorageIntegrationIfNeededInCurrentTxn(callCtx, catalog, integration); + // CATALOG_PRE_PERSIST transformation point + EntityTransformationEngine entityTransformationEngine = callCtx.getEntityTransformationEngine(); Review Comment: > For now, unfortunately we didn't quite manage to refactor all of the shared logic nryerrn AtomicOperationMetaStoreManager and TransactionalMetaStoreManagerImpl into a base class yet; the best we have is the prepareToPersistNewEntity hook defined in BaseMetaStoreManager. > > This means either this logic needs to be added in both the AtomicOperationMetaStoreManager and TransactionalMetaStoreManagerImpl, or needs to be abstracted into BaseMetaStoreManager somehow while making sure both of those impls invoke the shared logic. persistNewEntity isn't specific to catalog-creation so you'd need an enum on entityType there if you just put this logic in there. ########## polaris-core/src/main/java/org/apache/polaris/core/entity/transformation/EntityTransformationEngine.java: ########## @@ -0,0 +1,44 @@ +/* + * 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.entity.transformation; + +import org.apache.polaris.core.entity.PolarisBaseEntity; + +/** + * Engine responsible for applying a sequence of {@link EntityTransformer} transformations to a + * {@link PolarisBaseEntity}. + * + * <p>This abstraction allows Polaris to customize or enrich entities during runtime or persistence, + * based on configured or contextual logic (e.g., injecting service identity info, computing derived + * fields). + */ +public interface EntityTransformationEngine { + /** + * Applies all registered entity transformers to the provided entity, in order. + * + * @param transformationPoint The point in the entity lifecycle where transformers should be + * applied. + * @param entity The original Polaris entity to mutate. Review Comment: > It's too bad we don't have a way to express C++-style const correctness as we move towards immutable PolarisBaseEntity, but maybe we can at least mention here that the intention is that implementations should not mutate the input entity, but instead only create the new transformed entity. ########## quarkus/defaults/src/main/resources/application.properties: ########## @@ -188,6 +188,9 @@ polaris.oidc.principal-roles-mapper.type=default # polaris.storage.gcp.token=token # polaris.storage.gcp.lifespan=PT1H +# Polaris Entity Transformation Config +polaris.entity-transformation.transformers=no-op Review Comment: > On the one hand, it's useful to be able to have this kind of generalized transformer/mutator hook, but if the hook is also strongly coupled to particular features, we don't want to require separate configuration of the info injection here vs the core feature (e.g. sigv4) somewhere else. > > Especially if we end up having feature configurations for the core feature, then it's important that the feature itself auto-configures itself to be functional. In a way then the classes that define the sigv4 functionality should be what injects the mutator/transformer. -- 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: issues-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
