You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "vigneshio (via GitHub)" <gi...@apache.org> on 2026/07/08 08:13:09 UTC
[PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
vigneshio opened a new pull request, #5003:
URL: https://github.com/apache/polaris/pull/5003
Fixes a false-negative in the JDBC optimized location-overlap check (`OPTIMIZED_SIBLING_CHECK`).
When an ancestor location was stored in `location_without_scheme` without a trailing slash, the generated overlap query could not find it: the ancestor equality terms always ended in `/`, and the `LIKE` clause only matched descendants. The Java post-filter (`isChildOf`) never ran on the missing row, so nested table/namespace locations could be created under existing prefixes.
### Changes
- `QueryGenerator.generateOverlapQuery` now emits each ancestor prefix in both slash-terminated and non-slash-terminated forms.
- The descendant `LIKE` pattern is now slash-terminated so it matches true descendants (e.g. `//bucket/ns/tA/child`) without matching unrelated sibling prefixes (e.g. `//bucket/ns/tA_backup`).
- Updated `QueryGeneratorTest` expectations and added coverage for a location without a trailing slash.
- Added `testParentPrefixOverlapWithTrailingSlashMismatch` regression test in `AbstractLocalIcebergCatalogOverlapTest`.
## Checklist
- [x] Don't disclose security issues!
- [x] Clearly explained why
- [x] Added/updated tests
- [x] Updated CHANGELOG.md
- [ ] Updated site docs (if user-facing)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
vigneshio commented on PR #5003:
URL: https://github.com/apache/polaris/pull/5003#issuecomment-5225389184
> @vigneshio : Please resolve merge conflicts.
@dimas-b Resolved..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
vigneshio commented on PR #5003:
URL: https://github.com/apache/polaris/pull/5003#issuecomment-4996304567
> Thanks for bringing this PR to my attention.
>
> I'm not opposed to the changes, but I have two general questions before approving:
>
> 1. This PR seems to double the number of individual test cases inside the generated `IN` clause. I was wondering: shouldn't we instead remove the flag `ADD_TRAILING_SLASH_TO_LOCATION`, make it true by default, and instruct users to update their existing locations to always have a trailing slash? This would reduce the `IN` clause complexity from O(depth * 2) to O(depth).
> 2. While descendants can be easily queried with a `LIKE` statement, querying all ancestors requires N statements, where N is the number of path segments in the location. While this is reasonable for most paths, I wonder what happens when the path contains a huge number of segments. Would that break the SQL statement or make it inefficient? Could we maybe explore the idea of a different data type, e.g. [`ltree`](https://www.postgresql.org/docs/current/ltree.html) for PostgreSQL, associated with a `GIST` index? I'm no expert in this domain, but from a quick glance it seems that `ltree` type + `GIST` index would be a perfect match for the query patterns we need to support. I understand that this may not have an equivalent for, say, MySQL β so maybe the O(depth * 2) complexity of the `IN` clause is the least worst choice.
Thanks for taking a look, @adutra
`ADD_TRAILING_SLASH_TO_LOCATION` already defaults to true, so new locations get the trailing slash. The dual-form terms are only for backwards compat with locations stored before that (or with the flag off) - we can't really ask operators to rewrite existing rows without a migration. Path depth is small in practice, so O(depth * 2) is only a handful of extra terms.
[ltree](https://www.postgresql.org/docs/current/ltree.html) + `GIST` would be great for Postgres, but the JDBC backend also support **_H2/MySQL/CockroachDB_**, so we need something portable.. The term count is bounded by path depth, which stays small in real deployments. If deep paths ever become an issue, a DB-specific index could be a follow-up.
I think this PR is the least worst portable fix for the false-negative, and both ideas could be separate optimizations later. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b merged PR #5003:
URL: https://github.com/apache/polaris/pull/5003
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
vigneshio commented on PR #5003:
URL: https://github.com/apache/polaris/pull/5003#issuecomment-5001608316
> The fix looks right to me, and it's fixing a real false negative, isChildOf only drops candidates, it never adds one back, so a missed ancestor stays missed. I wonder how you're planning to fold this in with #5029 though, the two step on the same lines: this PR deletes the loop #5029 patches, and your new tests here still assert / and //, exactly what #5029 removes.
Plan is to land this one first since it's the correctness fix, then rebase #5029 on top. on the tests - yeah, this pr still emits `/` and `//` (they're on main too, this pr doesn't add them), and #5029 is where they get dropped, with the test updates. after this pr that skip gets really small - just a filter on the new` prefixTerms` loop (drop `/ `and `//`, keep `///` since it's a valid file: root, per @dimas-b point on #5029). kept them separate on purpose - different fixes (false-negative vs artifact terms) and #5023 is tracked on its own.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
vigneshio commented on code in PR #5003:
URL: https://github.com/apache/polaris/pull/5003#discussion_r3554457215
##########
persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/QueryGeneratorTest.java:
##########
@@ -365,34 +366,75 @@ void testGenerateOverlapQuery() {
-123L,
"/",
"//",
+ "//bucket",
"//bucket/",
+ "//bucket/tmp",
"//bucket/tmp/",
+ "//bucket/tmp/location",
+ "//bucket/tmp/location/",
+ "//bucket/tmp/location/%");
+
+ // A location without a trailing slash produces the same prefix terms so that ancestors stored
+ // in either form are matched.
+ Assertions.assertThatCollection(
+ QueryGenerator.generateOverlapQuery("realmId", 2, -123, "s3://bucket/tmp/location")
+ .parameters())
+ .containsExactly(
+ "realmId",
+ -123L,
+ "/",
+ "//",
Review Comment:
Agreed this looks odd. For scheme-stripped S3 paths we get `//bucket/...`, and
the prefix walk emits both `/` and `//`. Thatβs pre-existing (not introduced
here).
will follow-up if useful WDYT ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5003:
URL: https://github.com/apache/polaris/pull/5003#discussion_r3552655603
##########
persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/QueryGeneratorTest.java:
##########
@@ -365,34 +366,75 @@ void testGenerateOverlapQuery() {
-123L,
"/",
"//",
+ "//bucket",
"//bucket/",
+ "//bucket/tmp",
"//bucket/tmp/",
+ "//bucket/tmp/location",
+ "//bucket/tmp/location/",
+ "//bucket/tmp/location/%");
+
+ // A location without a trailing slash produces the same prefix terms so that ancestors stored
+ // in either form are matched.
+ Assertions.assertThatCollection(
+ QueryGenerator.generateOverlapQuery("realmId", 2, -123, "s3://bucket/tmp/location")
+ .parameters())
+ .containsExactly(
+ "realmId",
+ -123L,
+ "/",
+ "//",
Review Comment:
side note: this looks like a bug to me. Why would Polaris check for `/` and `//` at the same time? This is not even a root path π€
This is not a problem for this PR as it does not change this behaviour.
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogOverlapTest.java:
##########
@@ -294,4 +294,36 @@ public void testParentChildLocationOverlapWithOptimizedSiblingCheck() {
.hasMessageContaining("Unable to create entity at location")
.hasMessageContaining("conflicts with existing table or namespace");
}
+
+ @Test
+ public void testParentPrefixOverlapWithTrailingSlashMismatch() {
Review Comment:
This test is not specific, it does not fail without the changes in `QueryGenerator` π€
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #5003:
URL: https://github.com/apache/polaris/pull/5003#issuecomment-4996130679
Thanks for bringing this PR to my attention.
I'm not opposed to the changes, but I have two general questions before approving:
1. This PR seems to double the number of individual test cases inside the generated `IN` clause. I was wondering: shouldn't we instead remove the flag `ADD_TRAILING_SLASH_TO_LOCATION`, make it true by default, and instruct users to update their existing locations to always have a trailing slash? This would reduce the `IN` clause complexity from O(depth * 2) to O(depth).
2. While descendants can be easily queried with a `LIKE` statement, querying all ancestors requires N statements, where N is the number of path segments in the location. While this is reasonable for most paths, I wonder what happens when the path contains a huge number of segments. Would that break the SQL statement or make it inefficient? Could we maybe explore the idea of a different data type, e.g. [`ltree`](https://www.postgresql.org/docs/current/ltree.html) for PostgreSQL, associated with a `GIST` index? I'm no expert in this domain, but from a quick glance it seems that `ltree` type + `GIST` index would be a perfect match for the query patterns we need to support. I understand that this may not have an equivalent for, say, MySQL β so maybe the O(depth * 2) complexity of the `IN` clause is the least worst choice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5003:
URL: https://github.com/apache/polaris/pull/5003#discussion_r3554794222
##########
persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/QueryGeneratorTest.java:
##########
@@ -365,34 +366,75 @@ void testGenerateOverlapQuery() {
-123L,
"/",
"//",
+ "//bucket",
"//bucket/",
+ "//bucket/tmp",
"//bucket/tmp/",
+ "//bucket/tmp/location",
+ "//bucket/tmp/location/",
+ "//bucket/tmp/location/%");
+
+ // A location without a trailing slash produces the same prefix terms so that ancestors stored
+ // in either form are matched.
+ Assertions.assertThatCollection(
+ QueryGenerator.generateOverlapQuery("realmId", 2, -123, "s3://bucket/tmp/location")
+ .parameters())
+ .containsExactly(
+ "realmId",
+ -123L,
+ "/",
+ "//",
Review Comment:
yes, thx: #5023
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #5003:
URL: https://github.com/apache/polaris/pull/5003#issuecomment-5026466065
`dev` thread for visibility: https://lists.apache.org/thread/354rvrfoxrnwhnvdlp29d9zgydvncd37
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #5003:
URL: https://github.com/apache/polaris/pull/5003#issuecomment-5020294643
> I'm a bit confused... Are we ok to merge this "as is"? π€
Sorry, that's my fault. I won't approve myself because I have minor reservations; but I'm OK if you merge this as is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
vigneshio commented on code in PR #5003:
URL: https://github.com/apache/polaris/pull/5003#discussion_r3551380195
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java:
##########
@@ -390,18 +395,37 @@ public static PreparedQuery generateOverlapQuery(
List<String> conditions = new ArrayList<>();
List<Object> parameters = new ArrayList<>();
- String[] components = locationWithoutScheme.split("/");
+ // Normalize by stripping any trailing slash so the component loop does not produce an empty
+ // final component. The slash-terminated form is re-added as the last prefix term below.
+ String normalizedLocation = locationWithoutScheme;
+ if (normalizedLocation.endsWith("/")) {
+ normalizedLocation = normalizedLocation.substring(0, normalizedLocation.length() - 1);
+ }
+
+ String[] components = normalizedLocation.split("/");
StringBuilder pathBuilder = new StringBuilder();
+ Set<String> prefixTerms = new LinkedHashSet<>();
for (String component : components) {
- pathBuilder.append(component).append("/");
+ pathBuilder.append(component);
+ String prefixWithoutSlash = pathBuilder.toString();
+ if (!prefixWithoutSlash.isEmpty()) {
+ prefixTerms.add(prefixWithoutSlash);
+ }
+ pathBuilder.append("/");
+ prefixTerms.add(pathBuilder.toString());
Review Comment:
Good pointt.. π― - I've updated `generateOverlapQuery` to use `normalizedLocation.substring()` at each slash boundary instead. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #5003:
URL: https://github.com/apache/polaris/pull/5003#issuecomment-4938205698
Let's collect some more reviews since it's not a trivial area of the codebase :sweat_smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5003:
URL: https://github.com/apache/polaris/pull/5003#discussion_r3545501757
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java:
##########
@@ -390,18 +395,37 @@ public static PreparedQuery generateOverlapQuery(
List<String> conditions = new ArrayList<>();
List<Object> parameters = new ArrayList<>();
- String[] components = locationWithoutScheme.split("/");
+ // Normalize by stripping any trailing slash so the component loop does not produce an empty
+ // final component. The slash-terminated form is re-added as the last prefix term below.
+ String normalizedLocation = locationWithoutScheme;
+ if (normalizedLocation.endsWith("/")) {
+ normalizedLocation = normalizedLocation.substring(0, normalizedLocation.length() - 1);
+ }
+
+ String[] components = normalizedLocation.split("/");
StringBuilder pathBuilder = new StringBuilder();
+ Set<String> prefixTerms = new LinkedHashSet<>();
for (String component : components) {
- pathBuilder.append(component).append("/");
+ pathBuilder.append(component);
+ String prefixWithoutSlash = pathBuilder.toString();
+ if (!prefixWithoutSlash.isEmpty()) {
+ prefixTerms.add(prefixWithoutSlash);
+ }
+ pathBuilder.append("/");
+ prefixTerms.add(pathBuilder.toString());
Review Comment:
`prefixTerms` are all sub-strings of `normalizedLocation`, are they not? Could we use `String.substring()` instead of rebuilding/copying?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
vigneshio commented on code in PR #5003:
URL: https://github.com/apache/polaris/pull/5003#discussion_r3554481466
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogOverlapTest.java:
##########
@@ -294,4 +294,36 @@ public void testParentChildLocationOverlapWithOptimizedSiblingCheck() {
.hasMessageContaining("Unable to create entity at location")
.hasMessageContaining("conflicts with existing table or namespace");
}
+
+ @Test
+ public void testParentPrefixOverlapWithTrailingSlashMismatch() {
Review Comment:
@dimas-b Good catch. The default trailing slash meant the old check accidentally matched, so the test passed for the wrong reason. Fixed now.
PTAL!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #5003:
URL: https://github.com/apache/polaris/pull/5003#issuecomment-5223879949
@vigneshio : Please resolve merge conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #5003:
URL: https://github.com/apache/polaris/pull/5003#issuecomment-5004518061
> I think this PR is the least worst portable fix for the false-negative, and both ideas could be separate optimizations later. WDYT?
TBH I still think that `ADD_TRAILING_SLASH_TO_LOCATION` looks clumsy and should be removed.
> we can't really ask operators to rewrite existing rows without a migration
Why not? For example, https://github.com/apache/polaris/pull/4981 is introducing a migration script.
> [ltree](https://www.postgresql.org/docs/current/ltree.html) + GIST would be great for Postgres, but [...] we need something portable..
Agreed β I was just thinking out loud.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #5003:
URL: https://github.com/apache/polaris/pull/5003#issuecomment-5007374513
I'm a bit confused... Are we ok to merge this "as is"? :thinking:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #5003:
URL: https://github.com/apache/polaris/pull/5003#issuecomment-5026415583
Thanks for the update and review, @adutra !
I tend to think that the correctness fix is worth the extra queries load. I believe in most practical cases the namespace nesting depth is small enough for the database to be able to handle the `IN` clause with the current data structure.
+1 to further optimizations in this area.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org