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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
- OPA authorizer now includes the realm identifier in the authorization context sent to OPA (`input.context.realm`). This ensures OPA policies can enforce tenant isolation across realms, preventing potential collisions if identical principal or resource names exist in different realms.
- Management API delete operations for principals, principal roles, catalog roles, and catalogs now return error messages that match the actual failure reason (for example, concurrent modification no longer reports a misleading protected-entity message).
- Python CLI `setup apply` now defaults to an `INTERNAL` catalog type when the `type` field is left blank or null in the setup config, instead of crashing with `AttributeError`
- JDBC optimized location-overlap queries no longer include slash-only prefix terms such as `/` and `//`. Those were artifacts of stripping the URI scheme (e.g. `s3://bucket/path` → `//bucket/path`) and are not meaningful storage locations.
Comment thread
vigneshio marked this conversation as resolved.
Outdated

## [1.6.0]

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,9 @@ static PreparedQuery generateEntityTableExistQuery() {
*
* <p>Equality terms are generated for each prefix of the location in both slash-terminated and
* non-slash-terminated forms so that ancestors stored with or without a trailing slash are both
* matched.
* matched. Prefix terms that consist only of {@code /} characters (other than {@code ///}, the
* root of {@code file:} URIs) are skipped, as they are artifacts of scheme stripping, not
* meaningful storage locations.
*
* @param realmId A realm to search within
* @param schemaVersion The schema version of entities table to query
Expand Down Expand Up @@ -432,6 +434,11 @@ public static PreparedQuery generateOverlapQuery(
prefixTerms.add(normalizedLocation + "/");

for (String prefix : prefixTerms) {
// Skip "/" and "//" produced from empty path segments around the scheme separator; those
// are not meaningful storage locations. "///" (the root of file: URIs) is kept.
if (prefix.length() < 3 && isSlashOnly(prefix)) {
continue;
}
conditions.add("location_without_scheme = ?");
parameters.add(prefix);
Comment thread
vigneshio marked this conversation as resolved.
}
Expand All @@ -441,7 +448,6 @@ public static PreparedQuery generateOverlapQuery(
// //bucket/ns/tA_backup).
conditions.add("location_without_scheme LIKE ?");
parameters.add(StorageLocation.ensureTrailingSlash(locationWithoutScheme) + "%");

String locationClause = String.join(" OR ", conditions);
String clause = " WHERE realm_id = ? AND catalog_id = ? AND (" + locationClause + ")";

Expand All @@ -461,6 +467,20 @@ public static PreparedQuery generateOverlapQuery(
return new PreparedQuery(query.sql(), where.parameters());
}

/** True when {@code value} is non-empty and contains only {@code /} characters. */
@VisibleForTesting
static boolean isSlashOnly(String value) {
if (value == null || value.isEmpty()) {
return false;
}
for (int i = 0; i < value.length(); i++) {
if (value.charAt(i) != '/') {
return false;
}
}
return true;
}

static String getFullyQualifiedTableName(String tableName) {
// TODO: make schema name configurable.
return "POLARIS_SCHEMA." + tableName;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -317,23 +317,22 @@ void testGenerateWhereClauseExtended_allPredicatesAndStableParameterOrder() {

@Test
void testGenerateOverlapQuery() {
// s3://bucket/tmp/location/ → withoutScheme //bucket/tmp/location/
// Slash-only prefixes "/" and "//" from the scheme separator are not emitted.
assertEquals(
"SELECT id, catalog_id, parent_id, type_code, name, entity_version, sub_type_code,"
+ " create_timestamp, drop_timestamp, purge_timestamp, to_purge_timestamp, last_update_timestamp,"
+ " properties, internal_properties, grant_records_version, location_without_scheme FROM"
+ " POLARIS_SCHEMA.ENTITIES WHERE realm_id = ? AND catalog_id = ? AND (location_without_scheme = ?"
+ " OR location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme = ? OR"
+ " location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme = ? OR"
+ " location_without_scheme = ? OR location_without_scheme LIKE ?)",
+ " location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme LIKE ?)",
QueryGenerator.generateOverlapQuery("realmId", 2, -123, "s3://bucket/tmp/location/").sql());
Assertions.assertThatCollection(
QueryGenerator.generateOverlapQuery("realmId", 2, -123, "s3://bucket/tmp/location/")
.parameters())
.containsExactly(
"realmId",
-123L,
"/",
"//",
"//bucket",
"//bucket/",
"//bucket/tmp",
Expand All @@ -350,8 +349,6 @@ void testGenerateOverlapQuery() {
.containsExactly(
"realmId",
-123L,
"/",
"//",
"//bucket",
"//bucket/",
"//bucket/tmp",
Expand All @@ -360,22 +357,21 @@ void testGenerateOverlapQuery() {
"//bucket/tmp/location/",
"//bucket/tmp/location/%");

// Absolute path becomes file:///tmp/location/ → withoutScheme ///tmp/location/
// Slash-only prefixes "/" and "//" are not emitted; "///" (the file: root) is kept.
assertEquals(
"SELECT id, catalog_id, parent_id, type_code, name, entity_version, sub_type_code,"
+ " create_timestamp, drop_timestamp, purge_timestamp, to_purge_timestamp, last_update_timestamp,"
+ " properties, internal_properties, grant_records_version, location_without_scheme FROM"
+ " POLARIS_SCHEMA.ENTITIES WHERE realm_id = ? AND catalog_id = ? AND (location_without_scheme = ?"
+ " OR location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme = ? OR"
+ " location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme = ? OR"
+ " location_without_scheme LIKE ?)",
+ " location_without_scheme = ? OR location_without_scheme LIKE ?)",
QueryGenerator.generateOverlapQuery("realmId", 2, -123, "/tmp/location/").sql());
Assertions.assertThatCollection(
QueryGenerator.generateOverlapQuery("realmId", 2, -123, "/tmp/location/").parameters())
.containsExactly(
"realmId",
-123L,
"/",
"//",
"///",
"///tmp",
"///tmp/",
Expand All @@ -388,24 +384,41 @@ void testGenerateOverlapQuery() {
+ " create_timestamp, drop_timestamp, purge_timestamp, to_purge_timestamp, last_update_timestamp,"
+ " properties, internal_properties, grant_records_version, location_without_scheme"
+ " FROM POLARIS_SCHEMA.ENTITIES WHERE realm_id = ? AND catalog_id = ? AND (location_without_scheme = ?"
+ " OR location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme = ? OR"
+ " location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme LIKE ?)",
+ " OR location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme LIKE ?)",
QueryGenerator.generateOverlapQuery("realmId", 2, -123, "s3://バケツ/\"loc.ation\"/").sql());
Assertions.assertThatCollection(
QueryGenerator.generateOverlapQuery("realmId", 2, -123, "s3://バケツ/\"loc.ation\"/")
.parameters())
.containsExactly(
"realmId",
-123L,
"/",
"//",
"//バケツ",
"//バケツ/",
"//バケツ/\"loc.ation\"",
"//バケツ/\"loc.ation\"/",
"//バケツ/\"loc.ation\"/%");
}

@Test
void generateOverlapQueryDoesNotEmitSlashOnlyPrefixes() {
Comment thread
vigneshio marked this conversation as resolved.
// s3-style locations: "/" and "//" artifacts of scheme stripping are not emitted.
Assertions.assertThat(
QueryGenerator.generateOverlapQuery("realmId", 2, -123, "s3://bucket/ns/t/")
.parameters())
.doesNotContain("/", "//");
// Same without a trailing slash (prefix walk normalizes; slash-only skip is unchanged).
Assertions.assertThat(
QueryGenerator.generateOverlapQuery("realmId", 2, -123, "s3://bucket/ns/t")
.parameters())
.doesNotContain("/", "//");
// file: locations: the "///" root is kept, but "/" and "//" are not emitted.
Assertions.assertThat(
QueryGenerator.generateOverlapQuery("realmId", 2, -123, "file:///tmp/data/")
.parameters())
.contains("///")
.doesNotContain("/", "//");
}

@Test
void testGenerateExistsQuery() {
Map<String, Object> params = new HashMap<>();
Expand Down
Loading