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.
> > > >
> > >
>
