Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
- Fixed JDBC persistence under `SERIALIZABLE` isolation (e.g. CockroachDB default) so that a concurrent entity create that loses a unique-name race no longer returns the phantom new entity as a successful create. The conflicting row is now reported as `ENTITY_ALREADY_EXISTS` instead of fabricating the entity that was not persisted.
- Python CLI `setup` now preserves `endpoint_internal` and `sts_endpoint` during apply and export for S3 configuration
- Fixed a false-negative in the JDBC optimized location-overlap check (`OPTIMIZED_SIBLING_CHECK`). Ancestor locations stored in `location_without_scheme` without a trailing slash were not matched by the generated ancestor equality terms, allowing nested table/namespace locations to be created under existing prefixes. The query now emits both slash-terminated and non-slash-terminated prefix terms and uses a slash-terminated `LIKE` pattern for descendant matching.
- 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.

### Commits

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