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