Hi Eric, Thanks for the recap. It makes sense.
Perhaps we could rename the method (and the feature flag) to make it clear that it extends the functionality beyond mere optimizations. Cheers, Dmitri. On Wed, Sep 16, 2026 at 4:55 PM Eric Maynard <[email protected]> wrote: > My recollection is that the behavior change was an intentional improvement. > > The original sibling check was limited by the metastore implementation at > the time, and unfortunately exposed the catalog to security issues under > certain configurations. > > The optimized check — perhaps it should have been called a “v2” check or > something — corrected this mistake and implemented for the first time a > thorough sibling check. It seems like it would be a mistake to regress back > to an incomplete check just for consistency with the original flawed > implementation. > > —EM > > On Wed, Sep 16, 2026 at 1:16 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. > > > > > >
