Hi all, Yes, catalog-wide overlap checking was an improvement Eric introduced, and I think we should preserve that behavior.
We should clearly document the difference in coverage with the flag on and off, including when catalog-wide checking is needed. Calling it “optimized” suggests a performance-only change, which is misleading. I also agree we should clean up the configuration if needed, whether by giving the flag a clearer name or simplifying the related options. Yufei On Sat, Sep 19, 2026 at 2:01 AM Prithvi S <[email protected]> wrote: > Hi all, > > JB: yes, that is the shape I had in mind. > > Dmitri: those two tables do not require overlapping namespace locations, > and they cannot appear in a catalog that keeps the structured layout. They > need ALLOW_UNSTRUCTURED_TABLE_LOCATION. > > With the default structured constraint, you are right. Table locations are > confined to the parent namespace, and sibling namespaces are compared on > create, so ns1.t cannot sit on a path that overlaps ns2.t unless the > namespaces themselves overlap. The example is the case where that > confinement is turned off. > > Fresh catalog, overlap protection still on, optimized check still off: > 1. Catalog warehouse: s3://bucket/warehouse/ > 2. CREATE NAMESPACE ns1 -> s3://bucket/warehouse/ns1/ > 3. CREATE NAMESPACE ns2 -> s3://bucket/warehouse/ns2/ > Flag-off compares those two namespace locations. No overlap. Succeeds. > 4. ALLOW_UNSTRUCTURED_TABLE_LOCATION=true > 5. CREATE TABLE ns1.t LOCATION 's3://bucket/warehouse/shared/' > Flag-off lists entities under ns1. Does not see ns2. Succeeds. > 6. CREATE TABLE ns2.t LOCATION 's3://bucket/warehouse/shared/newchild/' > Flag-off lists entities under ns2. Does not see ns1.t. Succeeds (200). > Flag-on queries the catalog location index, finds ns1.t, returns 403. > > ns1.t is not inside ns2. ns2 is still at warehouse/ns2/. The tables left > the namespace tree; the namespaces did not overlap. > I will add a test for this sequence. The existing > testParentChildOverlapWithOptimizedSiblingCheck is the same-namespace > parent/child case with the flag on, not this cross-namespace escape. > > On the proposal I am in the same place as before, and with JB on the > follow-ups: > - Do not shrink hasOverlappingSiblings() to true siblings. That drops the > catalog-wide coverage from #1686 and #4873, including a foreign occupant on > a parent path. > - Land #5520 as the false-positive fix: skip own ancestors and the entity > itself, keep prefix search. > - Add a production-readiness warning when ALLOW_UNSTRUCTURED_TABLE_LOCATION > or DEFAULT_LOCATION_OBJECT_STORAGE_PREFIX_ENABLED is on and > OPTIMIZED_SIBLING_CHECK is left off. That combination is silent today. The > prefix helper already refuses it; unstructured without the prefix does not. > - Treat "optimized" as the naming bug. A new key with the old one as a > deprecated alias is the right follow-up and should not wait. > > The default being the weaker check is real. The remedy is to document and > warn, not to make the indexed path match the name. > > Thanks, > Prithvi S > > On Thu, Sep 17, 2026 at 11:15 AM Jean-Baptiste Onofré <[email protected]> > wrote: > > > 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. > > > > > > > > > > > >
