fix(jdbc): skip slash-only prefixes in optimized overlap queries - #5029
fix(jdbc): skip slash-only prefixes in optimized overlap queries#5029vigneshio wants to merge 5 commits into
Conversation
2c6a7b8 to
b513ba6
Compare
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
LGTM overall with minor comments 👍
f1efcb6 to
ee55051
Compare
flyingImer
left a comment
flyingImer
left a comment
There was a problem hiding this comment.
One blocking concern remains around the slash-only prefix filtering: it can hide an existing custom-location ancestor from the overlap check, as detailed inline. Once addressed, this only needs a rebase for the CHANGELOG conflict and fresh CI.
| // 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); |
There was a problem hiding this comment.
IIRC, Polaris still supports custom namespace locations outside the parent hierarchy through the ALLOW_NAMESPACE_CUSTOM_LOCATION compatibility flag. In that mode, s3:// is accepted and persisted as // in location_without_scheme, while S3Location treats s3://bucket/path as its child. Dropping // here means JDBC never loads that ancestor, so isChildOf cannot reject the overlap. Could we either retain this term and add a relational test for this pair, or reject scheme-root locations and handle existing rows before pruning it?
There was a problem hiding this comment.
Kept // and added the relational coverage.
The skip now drops only /. We retain // and /// since // can represent a valid namespace at the s3:// root in ALLOW_NAMESPACE_CUSTOM_LOCATION mode.
Added a test to ensure s3://bucket/ns/t still matches an ancestor at //. PTAL.
There was a problem hiding this comment.
Using s3:// as a location would be really awkward, but it's fine to support it for backward compatibility with old Polaris versions.
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 apache#5023
ee55051 to
260d923
Compare
3 participants
Fixes #5023.
Follow-up from the side note on #5003.
When building JDBC optimized location-overlap queries, scheme stripping turns paths like
s3://bucket/tmp/locationinto//bucket/tmp/location. The prefix walk then emitted slash-only terms (/,//) that are not real storage locations.