Hi All, I wonder if we should make OPTIMIZED_SIBLING_CHECK settable at the Catalog level and enable it by default for _new_ catalogs?
WDYT? Cheers, Dmitri. On Tue, Sep 22, 2026 at 7:46 PM Dmitri Bourlatchkov <[email protected]> wrote: > Hi Prithvi, > > Thanks for the info! It helped me clarify this stuff in my head (I hope). > > I agree with the plan (your email, JB's points, Eric's email). > > I think we may want to rename the OPTIMIZED_SIBLING_CHECK flag for > clarity... or at least update its description to make it clear that it is > essential when "unstructured" locations are at play. Right now it mentions > "non-standard location layouts", but that seems a bit too weak and does not > link to the other flags that affect runtime behaviour. > > I'll try and re-review PR #5520 ASAP. Looking forward to more PRs :) > > Cheers, > Dmitri. > > On Sat, Sep 19, 2026 at 5:03 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. >> > > > > >> > > > >> > >> >
