From 2c6a7b83eaad39df771204013887659655aea1af Mon Sep 17 00:00:00 2001 From: vignesh_A Date: Fri, 10 Jul 2026 19:00:19 +0530 Subject: [PATCH] fix(jdbc): skip slash-only prefixes in optimized overlap queries Scheme stripping turns s3://bucket/path into //bucket/path, so the prefix walk previously emitted / and // equality terms that are not real storage locations. Skip slash-only prefixes when building the overlap query. Same for file:/// paths that produced /// alone. Fixes #5023 --- CHANGELOG.md | 1 + .../relational/jdbc/QueryGenerator.java | 26 +++++++++++++-- .../relational/jdbc/QueryGeneratorTest.java | 33 +++++++++++++------ 3 files changed, 48 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 67cb7313f10..d01dce5253e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,6 +88,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti - `TableCleanupTaskHandler` now clamps `TABLE_METADATA_CLEANUP_BATCH_SIZE` to at least 1. Previously a non-positive realm override caused an infinite loop (0) or `IllegalArgumentException` (<0) when splitting metadata files for cleanup. - Azure SAS tokens are now signed for the configured duration instead of a hardcoded 1 hour, so long jobs no longer fail with expired credentials. - `ManifestFileCleanupTaskHandler` now handles Iceberg v2 delete manifests in addition to data manifests. Previously, `DROP TABLE PURGE` on a v2 table that had been updated via merge-on-read DML left position-delete files and their manifests as orphans in object storage; the cleanup task failed silently because `ManifestFiles.read()` rejects delete manifests. +- 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. ## [1.5.0] diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java index 033835f644c..8e0f47e1544 100644 --- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java +++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java @@ -375,6 +375,10 @@ static PreparedQuery generateEntityTableExistQuery() { * a path on one storage type may give a false positive for overlapping with another storage type. * This should be combined with a check using `StorageLocation`. * + *

Prefix equality terms that consist only of {@code /} characters are skipped. Those terms are + * artifacts of scheme stripping (e.g. {@code s3://bucket/path} becomes {@code //bucket/path} and + * would otherwise emit {@code /} and {@code //}), not meaningful storage locations. + * * @param realmId A realm to search within * @param schemaVersion The schema version of entities table to query * @param catalogId A catalog entity to search within @@ -395,14 +399,18 @@ public static PreparedQuery generateOverlapQuery( for (String component : components) { pathBuilder.append(component).append("/"); + String prefix = pathBuilder.toString(); + // Skip "/", "//", "///", ... produced from empty path segments around the scheme separator. + if (isSlashOnly(prefix)) { + continue; + } conditions.add("location_without_scheme = ?"); - parameters.add(pathBuilder.toString()); + parameters.add(prefix); } // Add LIKE condition to match children conditions.add("location_without_scheme LIKE ?"); parameters.add(locationWithoutScheme + "%"); - String locationClause = String.join(" OR ", conditions); String clause = " WHERE realm_id = ? AND catalog_id = ? AND (" + locationClause + ")"; @@ -422,6 +430,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; diff --git a/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/QueryGeneratorTest.java b/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/QueryGeneratorTest.java index 38a06f2b5fa..b716f76bb31 100644 --- a/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/QueryGeneratorTest.java +++ b/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/QueryGeneratorTest.java @@ -349,13 +349,14 @@ void testGenerateUpdateQueryExtended_rejectsInvalidColumns() { @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 LIKE ?)", + + " OR 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/") @@ -363,36 +364,48 @@ void testGenerateOverlapQuery() { .containsExactly( "realmId", -123L, - "/", - "//", "//bucket/", "//bucket/tmp/", "//bucket/tmp/location/", "//bucket/tmp/location/%"); + // Absolute path becomes file:///tmp/location/ → withoutScheme ///tmp/location/ + // Slash-only prefixes "/", "//", "///" 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 LIKE ?)", + + " POLARIS_SCHEMA.ENTITIES WHERE realm_id = ? AND catalog_id = ? AND (location_without_scheme = ?" + + " OR 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/location/", "///tmp/location/%"); + .containsExactly("realmId", -123L, "///tmp/", "///tmp/location/", "///tmp/location/%"); 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 LIKE ?)", + + " 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\"/%"); + "realmId", -123L, "//バケツ/", "//バケツ/\"loc.ation\"/", "//バケツ/\"loc.ation\"/%"); + } + + @Test + void generateOverlapQueryDoesNotEmitSlashOnlyPrefixes() { + Assertions.assertThat( + QueryGenerator.generateOverlapQuery("realmId", 2, -123, "s3://bucket/ns/t/") + .parameters()) + .doesNotContain("/", "//", "///"); + Assertions.assertThat( + QueryGenerator.generateOverlapQuery("realmId", 2, -123, "file:///tmp/data/") + .parameters()) + .filteredOn(p -> p instanceof String) + .noneMatch(p -> QueryGenerator.isSlashOnly((String) p)); } }