Hi All, I reviewed PR [5520] and it looks good to me overall. Some minor comments remain.
Given the complexity of the issue, it would be nice to get more reviews, though. Note that the PR includes a Persistence SPI change now. [5520] https://github.com/apache/polaris/pull/5520 Thanks, 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. >> > > > > >> > > > >> > >> >
