Hi Prithvi, I totally support performing catalog-wide location overlap checks.
What does not sit well with me is the behaviour difference with and without the OPTIMIZED_SIBLING_CHECK flag. We spent considerable time making improvements to that "optimized" code path, which is not even "on" by default. It makes the impression that the overlap protection is strong, while it is not so by default. > existing table ns1.t @ s3://bucket/foo/ > new table ns2.t @ s3://bucket/foo/newchild I'd like to understand how it is possible for these tables to exist. Should we not have detected an overlap at the namespace level? ns1.t should be located inside ns2's location, which must be distinct from ns1's location, right? Do you have a (simple) scenario in a fresh catalog leading to this situation? Thanks, Dmitri. On Wed, Sep 16, 2026 at 4:17 PM Prithvi S <[email protected]> wrote: > Hi Dmitri, all, > > Thanks for opening this. The names really do invite the reading you > describe, and the mismatch is worth being explicit about rather than > papering over in https://github.com/apache/polaris/pull/5520 > > I agree the current naming is misleading. hasOverlappingSiblings(), > OPTIMIZED_SIBLING_CHECK, and the javadoc on PolarisMetaStoreManager all > talk about same-namespace siblings. The flag-off path in > LocalIcebergCatalog.validateNoLocationOverlap() really is a same-parent > list. If this were only a performance switch, those two paths should match. > > I don't think we should fix that by shrinking the indexed implementations > to true siblings :) > > The catalog-wide search is not later drift. > https://github.com/apache/polaris/pull/1686 already queried by catalog_id > with prefix equality plus a descendant LIKE, not by parent_id. The leftover > // realmId and parentId go first comment still sits above a catalog_id > bind. In-memory does the same full scan. The flag description already says > enabling or disabling it can change overlap-detection coverage for > non-standard location layouts, and > DEFAULT_LOCATION_OBJECT_STORAGE_PREFIX_ENABLED is documented as relying on > catalog-wide uniqueness. That difference is load-bearing. When > ALLOW_UNSTRUCTURED_TABLE_LOCATION is on, a table can sit outside its parent > namespace’s location tree. Same-parent listing cannot see: > > existing table ns1.t @ s3://bucket/foo/ > > new table ns2.t @ s3://bucket/foo/newchild/ > > > Those entities do not share a parent, so the flag-off path lets this > through. The indexed path is supposed to reject it. > applyDefaultLocationObjectStoragePrefix() treats unstructured locations + > overlap-prevention + the flag off as an illegal combination for exactly > that reason. > IcebergOverlappingTableTest.testParentChildOverlapWithOptimizedSiblingCheck > encodes the same contract. > > If we restricted hasOverlappingSiblings() to true siblings, we would: > • reopen the parent-prefix hole that > https://github.com/apache/polaris/pull/4873 closed on NoSQL (a foreign > occupant on a parent path must still conflict) > • make prefixed / unstructured table locations unenforceable without > turning overlap protection off > • change user-visible 403 vs 200 behaviour for deployments that already > enabled the flag for catalog-wide coverage > > So I would treat the two paths as two different checks, not as an > optimization that accidentally changed meaning: > • flag off: cheap same-parent list; sufficient when locations follow the > namespace tree > • flag on: indexed catalog-wide containment; required when locations may > escape that tree > > An “optimization” flag should not have been the name for that second check. > Changing the implementation now to match the name would be a mistake :) > https://github.com/apache/polaris/issues/5521 and > https://github.com/apache/polaris/pull/5520 are a different bug. The > location index should still visit parent prefixes, because a foreign entity > there is a real overlap. What it must not do is treat the new entity’s own > ancestor chain as a conflict when that ancestor only strictly contains the > new location, or treat same parentId + type + name as a 403 instead of the > later 409. Own parent is not a sibling; a stranger on a parent path is. > > What I think: > 1. Land the https://github.com/apache/polaris/pull/5520 false-positive fix > (skip own ancestors and the entity itself; keep prefix search). > 2. Fix the javadoc and flag text so they describe catalog-wide containment, > including that the flag-off path is narrower. > 3. Leave a rename of the method / flag for a follow-up if we want it. The > current names are user-facing, so I would rather document the real contract > than silently change it. > > WDYT? > > Thanks, > Prithvi S > > On Wed, Sep 16, 2026 at 8:30 PM Dmitri Bourlatchkov <[email protected]> > wrote: > > > Hi All, > > > > This came to the forefront during the review of PR [5520]. > > > > PolarisMetaStoreManager declares the hasOverlappingSiblings() method. > > > > Currently, all persistent implementations of this method search all > > entities within the catalog. > > > > However, the caller of this > > method, LocalIcebergCatalog.validateNoLocationOverlap(), invokes it only > > when the OPTIMIZED_SIBLING_CHECK flag is set. If the flag is _not_ > > set, LocalIcebergCatalog searches > > only among immediate siblings. > > > > I think this is a logical inconsistency. An "optimization" should not > alter > > the validation method's basic behaviour. > > > > I'd like to propose adjusting hasOverlappingSiblings() implementations to > > _only_ search among true siblings. > > > > WDYT? > > > > [5520] https://github.com/apache/polaris/pull/5520 > > > > Thanks, > > Dmitri. > > >
