Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce globally unique table locations #67

Closed
Show file tree
Hide file tree
Changes from 14 commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,8 @@ public List<PolarisEntityActiveRecord> lookupEntityActiveBatch(
entity.getParentId(),
entity.getName(),
entity.getTypeCode(),
entity.getSubTypeCode()));
entity.getSubTypeCode(),
entity.getLocation()));
}

@Override
Expand Down Expand Up @@ -690,4 +691,9 @@ public void rollback() {
session.getTransaction().rollback();
}
}

@Override
public boolean locationOverlapsWithExistingTableLike(@NotNull PolarisCallContext callContext, String location) {
return this.store.locationOverlapsWithExistingTableLike(localSession.get(), location);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -409,4 +409,33 @@ void deletePrincipalSecrets(EntityManager session, String clientId) {

session.remove(modelPrincipalSecrets);
}

boolean locationOverlapsWithExistingTableLike(EntityManager session, String location) {
if (location == null) {
return false;
}
diagnosticServices.check(session != null, "session_is_null");

return session
.createQuery(
"""
SELECT
1
FROM
ModelEntityActive
WHERE
location IS NOT NULL
AND typeCode = :table_code
AND (
location LIKE CONCAT(:location, '%')
OR :location LIKE CONCAT(location, '%')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the 2nd condition cause a full-table-scan?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as written it unfortunately will.

Of course, this is all dependent on the metastore manager implementation (and in this case the backing database's implementation).

In any event if this check was always being done, I think there are some easy ways to optimize this. But since the check is optional, we may need to check every path. I'm still testing ways to improve performance of this optional check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this problem a bit. It's a tricky-fun one ;)
Just brain-dumping some thoughts:

There are a few things that play a role here - respectively w/ locations in general.

I think we can rely on / as a separator. With that in mind, a location is a duple of "bucket" (S3/GCS bucket or ADSL fs-name) plus a list of path elements. If we distinguish parent directories from table directories, the check becomes easier. I.e. a separate metastore entity that is only used to track locations.

CREATE TABLE locations (
  bucket TEXT NOT NULL,  -- storage bucket, e.g. s3://bucket/
  path TEXT NOT NULL,   -- storage location path, e.g. my/path/to/my-table
  kind TEXT NOT NULL,    -- marker for "parent-directory" or "table-location"
  entity_id TEXT NULL   -- id of the table
);

When you want to add/check for a new location like s3://bucket/my/path/my-table, the following INSERTs could do the trick:

INSERT INTO locations (bucket, path, kind, entity_id) VALUES ( 's3://bucket', `my`, `parent`, NULL) ON CONFLICT DO NOTHING;
INSERT INTO locations (bucket, path, kind, entity_id) VALUES ( 's3://bucket', `my/path`, `parent`, NULL) ON CONFLICT DO NOTHING;
INSERT INTO locations (bucket, path, kind, entity_id) VALUES ( 's3://bucket', `my/my-table`, `table`, '1234');

If the last one fails -> location already used -> fail hard. If any of the parents did not succeed, verify that those are all kind = 'parent'.

I suspect, this needs some more thought around race conditions (two tables with conflicting locations).

)
""",
ModelEntityDropped.class)
.setParameter("location", location)
.setParameter("table_code", PolarisEntityType.TABLE_LIKE.getCode())
.getResultStream()
.findFirst()
.isPresent();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ public class PolarisConfiguration {
public static final String ALLOW_NAMESPACE_LOCATION_OVERLAP = "ALLOW_NAMESPACE_LOCATION_OVERLAP";
public static final String ALLOW_EXTERNAL_METADATA_FILE_LOCATION =
"ALLOW_EXTERNAL_METADATA_FILE_LOCATION";
public static final String ENFORCE_GLOBALLY_UNIQUE_TABLE_LOCATIONS =
"ENFORCE_GLOBALLY_UNIQUE_TABLE_LOCATIONS";
public static final String ENFORCE_TABLE_LOCATIONS_INSIDE_NAMESPACE_LOCATIONS =
"ENFORCE_TABLE_LOCATIONS_INSIDE_NAMESPACE_LOCATIONS";

public static final String ALLOW_OVERLAPPING_CATALOG_URLS = "ALLOW_OVERLAPPING_CATALOG_URLS";

Expand All @@ -39,6 +43,8 @@ public class PolarisConfiguration {
public static final boolean DEFAULT_ALLOW_TABLE_LOCATION_OVERLAP = false;
public static final boolean DEFAULT_ALLOW_EXTERNAL_METADATA_FILE_LOCATION = false;
public static final boolean DEFAULT_ALLOW_NAMESPACE_LOCATION_OVERLAP = false;
public static final boolean DEFAULT_ENFORCE_GLOBALLY_UNIQUE_TABLE_LOCATIONS = false;
public static final boolean DEFAULT_ENFORCE_TABLE_LOCATIONS_INSIDE_NAMESPACE_LOCATIONS = true;

private PolarisConfiguration() {}
}
37 changes: 37 additions & 0 deletions polaris-core/src/main/java/io/polaris/core/PolarisUtils.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Copyright (c) 2024 Snowflake Computing Inc.
*
* Licensed 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 io.polaris.core;

/**
* A collection of utilities for polaris-core
*/
public class PolarisUtils {

/**
* Given some path, append a `/` if it doesn't already end with one
*/
public static String terminateWithSlash(String path) {
if (path == null) {
return null;
} else if (!path.endsWith("/")) {
return path + "/";
} else {
return path;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public Builder(Namespace namespace) {
setName(namespace.level(namespace.length() - 1));
}

public Builder setBaseLocation(String baseLocation) {
private Builder setBaseLocation(String baseLocation) {
properties.put(PolarisEntityConstants.ENTITY_BASE_LOCATION, baseLocation);
return this;
}
Expand All @@ -82,6 +82,12 @@ public Builder setParentNamespace(Namespace namespace) {
return this;
}

public Builder setLocation(String location) {
super.setLocation(location);
setBaseLocation(location);
return this;
}

public NamespaceEntity build() {
return new NamespaceEntity(buildBase());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ public class PolarisBaseEntity extends PolarisEntityCore {
// current version for that entity, will be monotonically incremented
protected int grantRecordsVersion;

// The location of the entity
protected String location;

public int getSubTypeCode() {
return subTypeCode;
}
Expand Down Expand Up @@ -113,6 +116,14 @@ public String getProperties() {
return properties != null ? properties : EMPTY_MAP_STRING;
}

public String getLocation() {
return this.location;
}

public void setLocation(String location) {
this.location = location;
}

@JsonIgnore
public Map<String, String> getPropertiesAsMap() {
if (properties == null) {
Expand Down Expand Up @@ -239,6 +250,7 @@ public PolarisBaseEntity(PolarisBaseEntity entity) {
this.properties = entity.getProperties();
this.internalProperties = entity.getInternalProperties();
this.grantRecordsVersion = entity.getGrantRecordsVersion();
this.location = entity.getLocation();
}

/** Build the DTO for a new entity */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ private PolarisEntity(
@JsonProperty("properties") String properties,
@JsonProperty("internalProperties") String internalProperties,
@JsonProperty("entityVersion") int entityVersion,
@JsonProperty("grantRecordsVersion") int grantRecordsVersion) {
@JsonProperty("grantRecordsVersion") int grantRecordsVersion,
@JsonProperty("location") String location) {
super(catalogId, id, type, subType, parentId, name);
this.createTimestamp = createTimestamp;
this.dropTimestamp = dropTimestamp;
Expand All @@ -97,6 +98,7 @@ private PolarisEntity(
this.internalProperties = internalProperties;
this.entityVersion = entityVersion;
this.grantRecordsVersion = grantRecordsVersion;
this.location = location;
}

public PolarisEntity(
Expand All @@ -113,7 +115,7 @@ public PolarisEntity(
Map<String, String> properties,
Map<String, String> internalProperties,
int entityVersion,
int grantRecordsVersion) {
int grantRecordsVersion, String location) {
super(catalogId, id, type, subType, parentId, name);
this.createTimestamp = createTimestamp;
this.dropTimestamp = dropTimestamp;
Expand All @@ -123,6 +125,7 @@ public PolarisEntity(
this.setInternalPropertiesAsMap(internalProperties);
this.entityVersion = entityVersion;
this.grantRecordsVersion = grantRecordsVersion;
this.location = location;
}

public static PolarisEntity of(PolarisBaseEntity sourceEntity) {
Expand Down Expand Up @@ -184,6 +187,7 @@ public PolarisEntity(@NotNull PolarisBaseEntity sourceEntity) {
this.internalProperties = sourceEntity.getInternalProperties();
this.entityVersion = sourceEntity.getEntityVersion();
this.grantRecordsVersion = sourceEntity.getGrantRecordsVersion();
this.location = sourceEntity.getLocation();
}

@JsonIgnore
Expand All @@ -201,6 +205,11 @@ public NameAndId nameAndId() {
return new NameAndId(name, id);
}

@JsonIgnore
public String getLocation() {
return location;
}

@Override
public String toString() {
StringBuilder sb = new StringBuilder();
Expand All @@ -211,6 +220,7 @@ public String toString() {
sb.append(";type=" + getType());
sb.append(";subType=" + getSubType());
sb.append(";internalProperties=" + getInternalPropertiesAsMap());
sb.append(";location=" + getLocation());
return sb.toString();
}

Expand All @@ -232,7 +242,8 @@ public boolean equals(Object o) {
&& subTypeCode == that.subTypeCode
&& Objects.equals(name, that.name)
&& Objects.equals(properties, that.properties)
&& Objects.equals(internalProperties, that.internalProperties);
&& Objects.equals(internalProperties, that.internalProperties)
&& Objects.equals(location, that.location);
}

@Override
Expand All @@ -251,7 +262,8 @@ public int hashCode() {
properties,
internalProperties,
entityVersion,
grantRecordsVersion);
grantRecordsVersion,
location);
}

public static class Builder extends BaseBuilder<PolarisEntity, Builder> {
Expand Down Expand Up @@ -284,6 +296,7 @@ public abstract static class BaseBuilder<T extends PolarisEntity, B extends Base
protected Map<String, String> internalProperties;
protected int entityVersion;
protected int grantRecordsVersion;
protected String location;

protected BaseBuilder() {
this.catalogId = -1;
Expand All @@ -300,6 +313,7 @@ protected BaseBuilder() {
this.internalProperties = new HashMap<>();
this.entityVersion = 1;
this.grantRecordsVersion = 1;
this.location = null;
}

protected BaseBuilder(T original) {
Expand All @@ -317,6 +331,7 @@ protected BaseBuilder(T original) {
this.internalProperties = new HashMap<>(original.getInternalPropertiesAsMap());
this.entityVersion = original.entityVersion;
this.grantRecordsVersion = original.grantRecordsVersion;
this.location = original.location;
}

public abstract T build();
Expand All @@ -338,7 +353,8 @@ public PolarisEntity buildBase() {
properties,
internalProperties,
entityVersion,
grantRecordsVersion);
grantRecordsVersion,
location);
}

public B setCatalogId(long catalogId) {
Expand Down Expand Up @@ -419,5 +435,10 @@ public B setGrantRecordsVersion(int grantRecordsVersion) {
this.grantRecordsVersion = grantRecordsVersion;
return (B) this;
}

public B setLocation(String location) {
this.location = location;
return (B) this;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.persistence.Column;

import java.util.Objects;

public class PolarisEntityActiveRecord {
Expand All @@ -38,6 +40,10 @@ public class PolarisEntityActiveRecord {
// code representing the subtype of that entity
private final int subTypeCode;

// location of the entity
@Column(nullable = true)
private String location;

public long getCatalogId() {
return catalogId;
}
Expand Down Expand Up @@ -66,6 +72,10 @@ public int getSubTypeCode() {
return subTypeCode;
}

public String getLocation() {
return location;
}

public PolarisEntitySubType getSubType() {
return PolarisEntitySubType.fromCode(this.subTypeCode);
}
Expand All @@ -77,13 +87,15 @@ public PolarisEntityActiveRecord(
@JsonProperty("parentId") long parentId,
@JsonProperty("name") String name,
@JsonProperty("typeCode") int typeCode,
@JsonProperty("subTypeCode") int subTypeCode) {
@JsonProperty("subTypeCode") int subTypeCode,
@JsonProperty("location") String location) {
this.catalogId = catalogId;
this.id = id;
this.parentId = parentId;
this.name = name;
this.typeCode = typeCode;
this.subTypeCode = subTypeCode;
this.location = location;
}

/** Constructor to create the object with provided entity */
Expand All @@ -94,6 +106,7 @@ public PolarisEntityActiveRecord(PolarisBaseEntity entity) {
this.typeCode = entity.getTypeCode();
this.name = entity.getName();
this.subTypeCode = entity.getSubTypeCode();
this.location = entity.getLocation();
}

@Override
Expand All @@ -106,12 +119,13 @@ public boolean equals(Object o) {
&& parentId == that.parentId
&& typeCode == that.typeCode
&& subTypeCode == that.subTypeCode
&& Objects.equals(name, that.name);
&& Objects.equals(name, that.name)
&& Objects.equals(location, that.location);
}

@Override
public int hashCode() {
return Objects.hash(catalogId, id, parentId, name, typeCode, subTypeCode);
return Objects.hash(catalogId, id, parentId, name, typeCode, subTypeCode, location);
}

@Override
Expand All @@ -130,6 +144,9 @@ public String toString() {
+ typeCode
+ ", subTypeCode="
+ subTypeCode
+ ", location='"
+ location
+ '\''
+ '}';
}
}
Loading
Loading