adutra commented on code in PR #4023:
URL: https://github.com/apache/polaris/pull/4023#discussion_r3383462916
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java:
##########
@@ -18,35 +18,57 @@
*/
package org.apache.polaris.service.catalog.io;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
import java.util.Optional;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntityConstants;
import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo;
+import org.apache.polaris.core.storage.StorageConfigOverrideResolver;
public class FileIOUtil {
private FileIOUtil() {}
/**
- * Finds storage configuration information in the hierarchy of the resolved
storage entity.
+ * Finds storage configuration information in the hierarchy of the resolved
storage entity, with
+ * any nearest-ancestor {@code storage_name_override} applied to the
catalog's base config.
*
- * <p>This method starts at the "leaf" level (e.g., table) and walks
"upwards" through namespaces
- * in the hierarchy to the "root." It searches for the first entity
containing storage config
- * properties, identified using a key from {@link
- * PolarisEntityConstants#getStorageConfigInfoPropertyName()}.
+ * <p>This method walks the path leaf-to-root, locates the first entity
carrying {@code
+ * storage_configuration_info} (the catalog), and — if any closer ancestor
carries a {@code
+ * storage_name_override} — returns a synthetic copy of the base entity
whose serialized
+ * configuration has its {@code storageName} replaced. Callers that read the
resulting entity's
+ * internal properties (e.g., to stash on a purge task) therefore see the
effective config.
*
* @param resolvedStorageEntity the resolved entity wrapper containing the
hierarchical path
- * @return an {@link Optional} containing the entity with storage config, or
empty if not found
+ * @return an {@link Optional} containing the (possibly synthetic) entity
with the effective
+ * storage config, or empty if no entity in the chain has a base storage
config
*/
public static Optional<PolarisEntity> findStorageInfoFromHierarchy(
PolarisResolvedPathWrapper resolvedStorageEntity) {
- Optional<PolarisEntity> storageInfoEntity =
- resolvedStorageEntity.getRawFullPath().reversed().stream()
+ List<PolarisEntity> path = resolvedStorageEntity.getRawFullPath();
+ Optional<PolarisStorageConfigurationInfo> effective =
+ StorageConfigOverrideResolver.resolveEffectiveConfig(path);
+ if (effective.isEmpty()) {
+ return Optional.empty();
+ }
+ Optional<PolarisEntity> baseEntity =
+ path.reversed().stream()
.filter(
e ->
e.getInternalPropertiesAsMap()
.containsKey(PolarisEntityConstants.getStorageConfigInfoPropertyName()))
.findFirst();
- return storageInfoEntity;
+ if (baseEntity.isEmpty()) {
Review Comment:
+1, there is some overlap between this class and
`StorageConfigOverrideResolver`.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -157,6 +158,13 @@ public class IcebergCatalog extends
BaseMetastoreViewCatalog
private static final Joiner SLASH = Joiner.on("/");
+ /**
+ * User-facing property key on namespaces and tables that selects a
server-configured named
+ * storage profile to use for credential vending. Validated and translated
into the internal
+ * property {@link
PolarisEntityConstants#getStorageNameOverridePropertyName()}.
+ */
+ static final String POLARIS_STORAGE_NAME_PROPERTY = "polaris.storage.name";
Review Comment:
Just thinking out loud: if I get this correctly, we would be tying a
namespace or table property to a storage profile that must be declared in the
server configuration. IOW, the namespace or table in question becomes unusable
if the catalog configuration changes and does not contain the profile anymore.
Probably OK, but this requires some coordination between two personas: the user
that created the table, and the admin that manages the server configuration.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageNameValidator.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.storage;
+
+import java.util.regex.Pattern;
+import org.jspecify.annotations.Nullable;
+
+/**
+ * Validates and normalizes user-supplied {@code polaris.storage.name}
property values used to
+ * select a server-configured named storage profile for credential vending.
+ *
+ * <p>Blank input (null, empty, or whitespace-only) normalizes to {@code
null}, which callers
+ * interpret as "clear the override".
+ */
+public final class StorageNameValidator {
+
+ public static final int MAX_LENGTH = 128;
+
+ private static final Pattern VALID = Pattern.compile("^[a-zA-Z0-9_-]+$");
+
+ private StorageNameValidator() {}
+
+ /**
+ * Trim, validate, and normalize a candidate storage name.
+ *
+ * @param value raw user input
+ * @return the trimmed name if valid, or {@code null} if the input is blank
Review Comment:
What do we mean by "normalize"? I think it could be counter-intuitive if
Polaris changed a user-supplied identifier without notifying the user about the
change, even if it's just trimming spaces.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java:
##########
@@ -89,6 +92,44 @@ public boolean validatePrefix() {
public abstract StorageType getStorageType();
+ /**
+ * Return a copy of {@code base} with its {@code storageName} replaced by
{@code storageName}. All
+ * other fields are preserved. Used by the storage-name override path so
that an entity-level
+ * override flows through credential vending and into the per-backend cache
key without mutating
+ * the original config.
+ *
+ * @throws IllegalArgumentException if {@code base} is not one of the four
supported subtypes
+ */
+ public static PolarisStorageConfigurationInfo withStorageName(
Review Comment:
Conceptually, this should be an abstract instance method:
```java
public abstract PolarisStorageConfigurationInfo withStorageName(@Nullable
String storageName);
```
Each subclass would implement accordingly.
Then callers would do:
```java
PolarisStorageConfigurationInfo base = ...
return base.withStorageName("some-storage");
```
--
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]
