Hi guys, I think the scenario described by Dmitri only bites when a table's location is explicitly overridden and escapes its parent namespace's location tree (not when two namespaces themselves overlap). If ns1 and ns2 have the same parent, the flag-off path already compares their locations directly, so that case is consider anyway. I believe the issue only happens with ALLOW_UNSTRUCTURED_TABLE_LOCATION, where a table under ns2 can be given an arbitrary location that happens to go inside ns1 substree. At that point, the table and ns1 aren't siblings by "parent link", only by location, and the flag off same-parent listing has no way to see it.
Prithvi, is that the shape of the repro you had in mind? If so, it would be worth to have a concrete test case, since it makes the "these are two different checks, not one optimized version of the other" argument much easier to see imho :) On the actual proposal, I'm with Prithvi and Eric here. Narrowing hasOverlappingSiblings() to true siblings to match the name would silently reopen the previous issue we had :) (see PR #4673), and it's the kind of change that's easy to justify an naming grounds but hard to notice as a regression. I would rather live with a confusing name flag rather than a correct name but weaker check :) I would add two follow ups: 1. ProductionReadinessChecks already warns when OPTIMIZED_SIBLINGS_CHECK is turned on without ALLOW_OPTIMIZED_SIBLING_CHECK (risky), but there's no warn for the opposite: ALLOW_UNSTRUCTURED_TABLE_LOCATION or DEFAULT_LOCATION_OBJECT_STORE_PREFIX_ENABLED enabled while OPTIMIZED_SIBLING_CHECK is left off (the default). That's the combination where the weaker path matters, and today it's just silent. Imho it's worth a readiness check entry or at minimum a doc update. 2. Regarding the name, I would see "optimized" as the actual bug here, separate from behavior. It reads as a perf hint but it actually change correctness coverage. I would support to introduce a "better" name as a non behavior change follow up (new key, old one deprecated/aliases) rather than leaving it for "someday": it's cheap, and I think it addresses Dmitri's oriiginal point without touching the semantics. Just a side note (I think I already shared that): I think we have wayyyyyyyy too much flags and configuration. Maybe after Polaris 1.8.0, we should start a review of flags that can be cleanup or set "implicitly" (instead of explictly which is always painful and confusing for our users). Regards JB On Wed, Sep 16, 2026 at 11:31 PM Dmitri Bourlatchkov <[email protected]> wrote: > > 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. > > > > >
