fix(jdbc): handle trailing-slash mismatch in optimized sibling overlap check

[vigneshio]

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.

Checklist

• Don't disclose security issues!
• Clearly explained why
• Added/updated tests
• Updated CHANGELOG.md
• Updated site docs (if user-facing)

[dimas-b]

Thanks for the fix, @vigneshio !

[dimas-b]

LGTM 👍 Thanks again, @vigneshio !

Please resolve conflicts.

[dimas-b]

Let's collect some more reviews since it's not a trivial area of the codebase 😅

[adutra]

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 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.

[vigneshio]

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 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 + 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?

[flyingImer]

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.

[vigneshio]

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.

[adutra]

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. (Not in this PR though.)

we can't really ask operators to rewrite existing rows without a migration

Why not? For example, #4981 is introducing a migration script.

ltree + GIST would be great for Postgres, but [...] we need something portable..

Agreed – I was just thinking out loud.

[dimas-b]

I'm a bit confused... Are we ok to merge this "as is"? 🤔