Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
### Breaking changes

- Concurrent table commits that hit a stale sequence number now return a retryable `409` instead of a fatal `400`, for both single-table commits and `commitTransaction`.
- Removed the `ADD_TRAILING_SLASH_TO_LOCATION` feature flag (catalog config
Comment thread
iprithv marked this conversation as resolved.
Outdated
`polaris.config.add-trailing-slash-to-location`). Polaris now always appends a trailing slash to
table and namespace base locations. A leftover key in configuration is harmless and ignored; a
production-readiness warning is emitted at startup if it is explicitly set to `false`. Locations
stored without a trailing slash by older versions continue to be handled correctly by
location-overlap checks.

### New Features

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -595,15 +595,6 @@ public static void enforceFeatureEnabledOrThrow(
.defaultValue(false)
.buildFeatureConfiguration();

public static final FeatureConfiguration<Boolean> ADD_TRAILING_SLASH_TO_LOCATION =
PolarisConfiguration.<Boolean>builder()
.key("ADD_TRAILING_SLASH_TO_LOCATION")
.catalogConfig("polaris.config.add-trailing-slash-to-location")
.description(
"When set, the base location for a table or namespace will have `/` added as a suffix if not present")
.defaultValue(true)
.buildFeatureConfiguration();

public static final FeatureConfiguration<Boolean> ALLOW_OPTIMIZED_SIBLING_CHECK =
PolarisConfiguration.<Boolean>builder()
.key("ALLOW_OPTIMIZED_SIBLING_CHECK")
Expand All @@ -625,9 +616,9 @@ public static void enforceFeatureEnabledOrThrow(
+ "views, and namespaces. This is not a bypass mode, but enabling or disabling "
+ "it can change overlap-detection coverage for non-standard location layouts. "
+ "Only enable it when the required index and backfill state is known to be "
+ "correct. For correct results, locations should end with a slash; see "
+ "ADD_TRAILING_SLASH_TO_LOCATION. Supported by the JDBC and NoSQL metastore "
+ "implementations.")
+ "correct. Locations written by Polaris always end with a slash; locations "
+ "stored by older versions without one are still handled. Supported by the "
+ "JDBC and NoSQL metastore implementations.")
.defaultValue(false)
.buildFeatureConfiguration();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -674,9 +674,7 @@ private void createNamespaceInternal(
String baseLocation = resolveNamespaceLocation(namespace, metadata);

// Set / suffix
boolean requireTrailingSlash =
realmConfig.getConfig(FeatureConfiguration.ADD_TRAILING_SLASH_TO_LOCATION);
if (requireTrailingSlash && !baseLocation.endsWith("/")) {
if (!baseLocation.endsWith("/")) {
baseLocation += "/";
Comment thread
iprithv marked this conversation as resolved.
Outdated
}

Expand Down Expand Up @@ -2818,10 +2816,7 @@ private void createTableLike(
boolean validateMetadataLocation) {
IcebergTableLikeEntity icebergTableLikeEntity = IcebergTableLikeEntity.of(entity);
// Set / suffix
boolean requireTrailingSlash =
realmConfig.getConfig(FeatureConfiguration.ADD_TRAILING_SLASH_TO_LOCATION);
if (requireTrailingSlash
&& icebergTableLikeEntity.getBaseLocation() != null
if (icebergTableLikeEntity.getBaseLocation() != null
&& !icebergTableLikeEntity.getBaseLocation().endsWith("/")) {
icebergTableLikeEntity =
new IcebergTableLikeEntity.Builder(icebergTableLikeEntity)
Expand Down Expand Up @@ -2887,10 +2882,7 @@ private void updateTableLike(
IcebergTableLikeEntity icebergTableLikeEntity = new IcebergTableLikeEntity(entity);

// Set / suffix
boolean requireTrailingSlash =
realmConfig.getConfig(FeatureConfiguration.ADD_TRAILING_SLASH_TO_LOCATION);
if (requireTrailingSlash
&& icebergTableLikeEntity.getBaseLocation() != null
if (icebergTableLikeEntity.getBaseLocation() != null
&& !icebergTableLikeEntity.getBaseLocation().endsWith("/")) {
icebergTableLikeEntity =
new IcebergTableLikeEntity.Builder(icebergTableLikeEntity)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ public class ProductionReadinessChecks {
private static final String REFLECTION_FREE_SERIALIZERS_PROPERTY =
"quarkus.rest.jackson.optimization.enable-reflection-free-serializers";

/**
* Key of the removed {@code ADD_TRAILING_SLASH_TO_LOCATION} feature flag, kept only to warn
* operators whose configuration still sets it.
*/
private static final String ADD_TRAILING_SLASH_TO_LOCATION_KEY = "ADD_TRAILING_SLASH_TO_LOCATION";

/**
* A warning sign ⚠ {@code 26A0} with variant selector {@code FE0F}. The sign is preceded by a
* null character {@code 0000} to ensure that the warning sign is displayed correctly regardless
Expand Down Expand Up @@ -371,6 +377,39 @@ public ProductionReadinessCheck checkOverlappingSiblingCheckSettings(
: ProductionReadinessCheck.of(errors.toArray(new Error[0]));
}

@Produces
public ProductionReadinessCheck checkAddTrailingSlashToLocation(
FeaturesConfiguration featureConfiguration) {
var message =
"ADD_TRAILING_SLASH_TO_LOCATION was removed and is ignored. Polaris always adds a "
+ "trailing slash to table and namespace base locations. Remove this setting.";
var errors = new ArrayList<Error>();
if ("false"
.equalsIgnoreCase(
featureConfiguration.defaults().get(ADD_TRAILING_SLASH_TO_LOCATION_KEY))) {
errors.add(
Error.of(message, format("polaris.features.\"%s\"", ADD_TRAILING_SLASH_TO_LOCATION_KEY)));
}
featureConfiguration
.realmOverrides()
.forEach(
(realmId, overrides) -> {
if ("false"
.equalsIgnoreCase(
overrides.overrides().get(ADD_TRAILING_SLASH_TO_LOCATION_KEY))) {
errors.add(
Error.of(
message,
format(
"polaris.features.realm-overrides.\"%s\".overrides.\"%s\"",
realmId, ADD_TRAILING_SLASH_TO_LOCATION_KEY)));
}
});
return errors.isEmpty()
? ProductionReadinessCheck.OK
: ProductionReadinessCheck.of(errors.toArray(new Error[0]));
}

@Produces
public ProductionReadinessCheck checkGcsPrincipalAttribution(
FeaturesConfiguration featureConfiguration) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,15 @@
import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.entity.CatalogEntity;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntityCore;
import org.apache.polaris.core.entity.PolarisEntitySubType;
import org.apache.polaris.core.entity.PolarisEntityType;
import org.apache.polaris.core.entity.table.IcebergTableLikeEntity;
import org.apache.polaris.core.identity.provider.ServiceIdentityProvider;
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.core.persistence.bootstrap.RootCredentialsSet;
import org.apache.polaris.core.persistence.dao.entity.EntityResult;
import org.apache.polaris.core.persistence.resolver.ResolutionManifestFactory;
import org.apache.polaris.core.persistence.resolver.ResolverFactory;
import org.apache.polaris.core.storage.PolarisStorageIntegrationProvider;
Expand Down Expand Up @@ -299,9 +304,11 @@ public void testParentChildLocationOverlapWithOptimizedSiblingCheck() {

@Test
public void testParentPrefixOverlapWithTrailingSlashMismatch() {
// Profiles disable ADD_TRAILING_SLASH_TO_LOCATION so the parent is stored without a trailing
// slash. That is the OPTIMIZED_SIBLING_CHECK false-negative for JDBC: ancestor equality terms
// used to be slash-terminated only, so location_without_scheme without '/' was missed.
// The catalog now always stores locations with a trailing slash
// (ADD_TRAILING_SLASH_TO_LOCATION was removed). Simulate legacy slash-less data
// by rewriting the stored parent location directly, then verify the optimized sibling check
// still detects overlaps against it: ancestor equality terms used to be slash-terminated only,
// so a slash-less location_without_scheme was missed.
Namespace ns = Namespace.of("ns-for-trailing-slash-overlap");
catalog().createNamespace(ns);

Expand All @@ -310,9 +317,11 @@ public void testParentPrefixOverlapWithTrailingSlashMismatch() {
assertThat(parentLoc).doesNotEndWith("/");
catalog().buildTable(parentTable, SCHEMA).withLocation(parentLoc).create();

// Guardrail: if trailing-slash normalization were still on, this test would not exercise the
// slash-less location_without_scheme path that QueryGenerator must handle.
assertThat(catalog().loadTable(parentTable).location())
stripStoredBaseLocationTrailingSlash(ns, parentTable);

// Guardrail: the stored location must be slash-less so overlap checks exercise the
// non-slash-terminated location_without_scheme path that QueryGenerator must handle.
assertThat(storedTableBaseLocation(ns, parentTable))
.as("parent must remain slash-less so overlap uses non-slash-terminated stored location")
.isEqualTo(parentLoc)
.doesNotEndWith("/");
Expand All @@ -335,4 +344,54 @@ public void testParentPrefixOverlapWithTrailingSlashMismatch() {
.hasMessageContaining("Unable to create entity at location")
.hasMessageContaining("conflicts with existing table or namespace");
}

private String storedTableBaseLocation(Namespace ns, TableIdentifier table) {
return IcebergTableLikeEntity.of(readStoredTableLike(ns, table)).getBaseLocation();
}

/**
* Rewrites the stored base location of {@code table} without its trailing slash, simulating data
* written before Polaris always appended one.
*/
private void stripStoredBaseLocationTrailingSlash(Namespace ns, TableIdentifier table) {
PolarisEntity tableEntity = readStoredTableLike(ns, table);
IcebergTableLikeEntity tableLike = IcebergTableLikeEntity.of(tableEntity);
String baseLocation = tableLike.getBaseLocation();
assertThat(baseLocation).endsWith("/");
IcebergTableLikeEntity stripped =
new IcebergTableLikeEntity.Builder(tableLike)
.setBaseLocation(baseLocation.substring(0, baseLocation.length() - 1))
.build();
EntityResult result =
metaStoreManager.updateEntityPropertiesIfNotChanged(
polarisContext, tableLikeCatalogPath(ns), stripped);
assertThat(result.isSuccess()).isTrue();
}

private PolarisEntity readStoredTableLike(Namespace ns, TableIdentifier table) {
EntityResult result =
metaStoreManager.readEntityByName(
polarisContext,
tableLikeCatalogPath(ns),
PolarisEntityType.TABLE_LIKE,
PolarisEntitySubType.ICEBERG_TABLE,
table.name());
assertThat(result.isSuccess()).isTrue();
return PolarisEntity.of(result.getEntity());
}

private List<PolarisEntityCore> tableLikeCatalogPath(Namespace ns) {
assertThat(ns.length()).as("test namespaces must be single-level").isEqualTo(1);
EntityResult result =
metaStoreManager.readEntityByName(
polarisContext,
List.of(PolarisEntity.toCore(catalogEntity)),
PolarisEntityType.NAMESPACE,
PolarisEntitySubType.NULL_SUBTYPE,
ns.level(0));
assertThat(result.isSuccess()).isTrue();
return List.of(
PolarisEntity.toCore(catalogEntity),
PolarisEntity.toCore(PolarisEntity.of(result.getEntity())));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,6 @@ public Map<String, String> getConfigOverrides() {
overrides.put("polaris.features.\"ALLOW_TABLE_LOCATION_OVERLAP\"", "false");
overrides.put("polaris.features.\"OPTIMIZED_SIBLING_CHECK\"", "true");
overrides.put("polaris.features.\"ALLOW_OPTIMIZED_SIBLING_CHECK\"", "true");
// Keep locations as written so slash-less base locations are stored as-is.
overrides.put("polaris.features.\"ADD_TRAILING_SLASH_TO_LOCATION\"", "false");
return overrides;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,6 @@ public Map<String, String> getConfigOverrides() {
overrides.put("polaris.features.\"ALLOW_TABLE_LOCATION_OVERLAP\"", "false");
overrides.put("polaris.features.\"OPTIMIZED_SIBLING_CHECK\"", "true");
overrides.put("polaris.features.\"ALLOW_OPTIMIZED_SIBLING_CHECK\"", "true");
// Keep locations as written so slash-less base locations are stored as-is.
overrides.put("polaris.features.\"ADD_TRAILING_SLASH_TO_LOCATION\"", "false");
return overrides;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,6 @@ public Map<String, String> getConfigOverrides() {
overrides.put("polaris.features.\"ALLOW_TABLE_LOCATION_OVERLAP\"", "false");
overrides.put("polaris.features.\"OPTIMIZED_SIBLING_CHECK\"", "true");
overrides.put("polaris.features.\"ALLOW_OPTIMIZED_SIBLING_CHECK\"", "true");
// Keep locations as written so JDBC optimized sibling checks exercise slash-less
// location_without_scheme values (the false-negative fixed in QueryGenerator).
overrides.put("polaris.features.\"ADD_TRAILING_SLASH_TO_LOCATION\"", "false");
overrides.put("polaris.persistence.type", "relational-jdbc");
overrides.put("polaris.persistence.auto-bootstrap-types", "relational-jdbc");
overrides.put("quarkus.datasource.db-kind", "h2");
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
/*
* 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.service.config;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.util.Map;
import org.apache.polaris.core.config.ProductionReadinessCheck;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.junit.jupiter.MockitoExtension;

@ExtendWith(MockitoExtension.class)
class ProductionReadinessChecksAddTrailingSlashTest {

private static final String FLAG_KEY = "ADD_TRAILING_SLASH_TO_LOCATION";

private ProductionReadinessChecks checks;

@BeforeEach
void setUp() {
checks = new ProductionReadinessChecks();
}

@Test
void flagUnsetReturnsOk() {
FeaturesConfiguration config = mockConfig(Map.of(), Map.of());

ProductionReadinessCheck result = checks.checkAddTrailingSlashToLocation(config);

assertThat(result.ready()).isTrue();
}

@Test
void flagExplicitlyTrueReturnsOk() {
FeaturesConfiguration config = mockConfig(Map.of(FLAG_KEY, "true"), Map.of());

ProductionReadinessCheck result = checks.checkAddTrailingSlashToLocation(config);

assertThat(result.ready()).isTrue();
}

@Test
void flagExplicitlyFalseReturnsWarning() {
FeaturesConfiguration config = mockConfig(Map.of(FLAG_KEY, "false"), Map.of());

ProductionReadinessCheck result = checks.checkAddTrailingSlashToLocation(config);

assertThat(result.ready()).isFalse();
assertThat(result.getErrors())
.singleElement()
.satisfies(
error -> {
assertThat(error.offendingProperty()).contains(FLAG_KEY);
assertThat(error.severe()).isFalse();
});
}

@Test
void realmOverrideFalseReturnsWarning() {
FeaturesConfiguration config =
mockConfig(Map.of(), Map.of("test-realm", mockRealmOverrides(Map.of(FLAG_KEY, "false"))));

ProductionReadinessCheck result = checks.checkAddTrailingSlashToLocation(config);

assertThat(result.ready()).isFalse();
assertThat(result.getErrors())
.singleElement()
.satisfies(
error -> {
assertThat(error.offendingProperty()).contains("test-realm").contains(FLAG_KEY);
assertThat(error.severe()).isFalse();
});
}

private static FeaturesConfiguration mockConfig(
Map<String, String> defaults, Map<String, RealmOverridable.RealmOverrides> realmOverrides) {
FeaturesConfiguration config = mock(FeaturesConfiguration.class);
when(config.defaults()).thenReturn(defaults);
when(config.realmOverrides()).thenReturn(realmOverrides);
return config;
}

private static RealmOverridable.RealmOverrides mockRealmOverrides(Map<String, String> overrides) {
RealmOverridable.RealmOverrides realmOverrides = mock(RealmOverridable.RealmOverrides.class);
when(realmOverrides.overrides()).thenReturn(overrides);
return realmOverrides;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,6 @@ build:

Feature configurations for Polaris. These are stable, user-facing settings.

##### `polaris.features."ADD_TRAILING_SLASH_TO_LOCATION"`

When set, the base location for a table or namespace will have `/` added as a suffix if not present

- **Type:** `Boolean`
- **Default:** `true`
- **Catalog Config:** `polaris.config.add-trailing-slash-to-location`

---

##### `polaris.features."ALLOW_CLIENT_SPECIFIED_TABLE_LOCATION"`

If set to true (the default), Polaris honors a `location` (and the `write.data.path` / `write.metadata.path` properties) explicitly supplied in a create or update request, subject to the usual structured-location, allowed-location, metadata-location, and overlap validation. If set to false, such requests are rejected, regardless of the other location compatibility flags. This setting does not apply to federated catalogs.
Expand Down Expand Up @@ -460,7 +450,7 @@ How many times to retry refreshing metadata when the previous error was retryabl

##### `polaris.features."OPTIMIZED_SIBLING_CHECK"`

When set, Polaris uses an index to perform sibling overlap checks between tables, views, and namespaces. This is not a bypass mode, but enabling or disabling it can change overlap-detection coverage for non-standard location layouts. Only enable it when the required index and backfill state is known to be correct. For correct results, locations should end with a slash; see ADD_TRAILING_SLASH_TO_LOCATION. Supported by the JDBC and NoSQL metastore implementations.
When set, Polaris uses an index to perform sibling overlap checks between tables, views, and namespaces. This is not a bypass mode, but enabling or disabling it can change overlap-detection coverage for non-standard location layouts. Only enable it when the required index and backfill state is known to be correct. Locations written by Polaris always end with a slash; locations stored by older versions without one are still handled. Supported by the JDBC and NoSQL metastore implementations.

- **Type:** `Boolean`
- **Default:** `false`
Expand Down
Loading